Skip to content

tests/mtd_raw: add simple test for MTD#15362

Merged
benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco:tests/mtd_raw
Jan 11, 2021
Merged

tests/mtd_raw: add simple test for MTD#15362
benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco:tests/mtd_raw

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Nov 2, 2020

Contribution description

There are several tests for file systems that use the MTD layer but there is currently no test to quickly test out the behavior of a MTD device.
This fixes that.

Testing procedure

Run tests/mtd_raw on a board with a MTD device configured.
You can manually read / write / erase data, but there is also a test command that performs some automatic tests to quickly verify the correct working of a MTD implementation.

Issues/PRs references

@benpicco benpicco requested a review from basilfx November 2, 2020 20:21
@benpicco benpicco added Area: tests Area: tests and testing framework Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Nov 2, 2020
@benpicco benpicco mentioned this pull request Nov 2, 2020
Merged
@benpicco benpicco requested a review from jue89 November 3, 2020 12:53
jue89
jue89 previously requested changes Nov 3, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jue89 jue89 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one! This comes in handy ... I'm currently working with MTDs, as well. Thank you!

As a first test, I tried your PR on native. To compile, I had to bring in some small changes:

jue89
jue89 reviewed Nov 3, 2020
View reviewed changes
@jue89
Copy link
Copy Markdown
Contributor

jue89 commented Nov 3, 2020

Hmm, > test 0 doesn't work on native:

jue@bart ~/P/RIOT ((c196f338…) ⚡↩☡) make -C tests/mtd_raw/ BOARD=native term
make: Entering directory '/home/jue/Projects/RIOT/tests/mtd_raw'
/home/jue/Projects/RIOT/tests/mtd_raw/bin/native/tests_mtd_raw.elf  
RIOT native interrupts/signals initialized.
LED_RED_OFF
LED_GREEN_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2021.01-devel-531-gc196f-HEAD)
Manual MTD test
init MTD_0… OK (8192 kiB)
> test 0
test 0
[START]
tests/mtd_raw/main.c:400 => 0x565a5913
*** RIOT kernel panic:
FAILED ASSERTION.

*** halted.

I'm not sure, what's wrong here and if this is related to your code. If no one is faster than me, I'll have a deeper look soon.

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Nov 3, 2020

I know what's happening: mtd_native does not implement the new pagewise API yet, so no unaligned writes / writes across page boundaries are possible.

basilfx
basilfx reviewed Nov 3, 2020
View reviewed changes
@benpicco benpicco mentioned this pull request Nov 3, 2020
Merged
@jue89 jue89 mentioned this pull request Nov 4, 2020
Closed
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 18, 2020
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 18, 2020
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Nov 24, 2020

@jue89 test should now also work on native

@benpicco benpicco requested a review from dylad November 26, 2020 21:58
fjmolinas
fjmolinas reviewed Dec 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an automatic test script?

fjmolinas
fjmolinas reviewed Dec 8, 2020
View reviewed changes
@benpicco benpicco requested a review from miri64 as a code owner December 12, 2020 23:08
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Dec 12, 2020

added both tests

@benpicco benpicco requested review from kaspar030 and maribu January 5, 2021 16:43
@benpicco benpicco force-pushed the tests/mtd_raw branch 2 times, most recently from 1756b5d to 776ca35 Compare January 5, 2021 17:03
maribu
maribu reviewed Jan 6, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, some comments inline

maribu
maribu reviewed Jan 7, 2021
View reviewed changes
maribu
maribu reviewed Jan 7, 2021
View reviewed changes
@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jan 7, 2021
@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 7, 2021

@jue89: Can you check whether your request for changes has been addressed?

miri64
miri64 reviewed Jan 8, 2021
View reviewed changes
@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 8, 2021

I tested this on the weact-f411ce with the flash footprint on the back populated. The automatic test worked as expected, and also playing a bit with the other shell commands seems to work as expected.

@maribu maribu added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jan 8, 2021
@maribu maribu dismissed jue89’s stale review January 8, 2021 09:50

I just went through every comment and can confirm that every requested change has been applied.

@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 8, 2021

@benpicco: Care to also sneak in a fix for the style issues in od.c the CI is very unhappy about? (for{ --> for { in line 44, if{ --> if { in line 45, else{ --> else { in line 49)

@miri64: Are your comments addressed? If so, I'd be ready to ACK :-)

@jia200x: I think an additional test can be considered to be of low impact and being merged during soft feature freeze, as the chances this breaks anything are pretty low. Do you you agree?

@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 8, 2021
@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 8, 2021

I'm fine with it!

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 8, 2021
maribu
maribu approved these changes Jan 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. But let's wait with hitting merge for @miri64 to confirm her request for changes are addressed.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 11, 2021

Yepp. Happy with the changes. :-)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 11, 2021

I think this needs a rebase to have the commit check changes in however.

@benpicco
sys/od: allow to specify address offset with od_hex_dump_ext()
89c97ae 
When dumping memory the printed addresses always start with `00000000`.
This can be very confusing and lead to errors.

Allow the user to specify a starting address of the printed memory that
will be used instead.

By introducing a wrapper function, existing users are unaffected.
@benpicco
tests/mtd_raw: add simple test for MTD
cf2ac21
@benpicco benpicco merged commit 08ef862 into RIOT-OS:master Jan 11, 2021
@benpicco benpicco deleted the tests/mtd_raw branch January 11, 2021 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 left review comments

@basilfx basilfx basilfx left review comments

@jue89 jue89 jue89 left review comments

@fjmolinas fjmolinas fjmolinas left review comments

@maribu maribu maribu approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@smlng smlng Awaiting requested review from smlng

@dylad dylad Awaiting requested review from dylad

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees

No one assigned

Labels

Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@benpicco @jue89 @maribu @jia200x @miri64 @basilfx @fjmolinas