drivers/flashpage: use const pointers for write and verify by vincent-d · Pull Request #8773 · RIOT-OS/RIOT

vincent-d · 2018-03-13T15:55:48Z

Contribution description

This update the periph_flashpage interface to use const pointer for writes and verify operations.

Issues/PRs references

Based on #8768

aabadie

ACK

vincent-d · 2018-03-13T20:53:44Z

Rebased

kYc0o · 2018-03-13T22:29:57Z

@cladmi can you take a look? I remember we talked something about this but decided to not go this way for a reason... I can't recall it right now...

cladmi · 2018-03-14T13:09:07Z

Going for const is good, as it only tells that functions do not modify the data, so I ACK this part. Its a better API with it.

However you removed the (uint32_t *) cast from the void *data instead of going for (const uint32_t *) and that should be reverted I think.

From what I remember, the goal of these lines was to somehow counter a lack in the api types saying that "data" should be 4 bytes aligned. Its a hack instead of a proper fix, but there is not need to remove it now.

cladmi

Please keep the (const uint32_t *) cast as explained in the comment.

cladmi · 2018-03-14T13:11:37Z

cpu/efm32/periph/flashpage.c

    assert(page < (int)FLASHPAGE_NUMOF);

    uint32_t *page_addr = (uint32_t *)flashpage_addr(page);
-    uint32_t *data_addr = (uint32_t *)data;


I think this should be kept as (const uint32_t *)data as it somehow says alignment constraints, even if its note in the API.

I did not repeat but its also present in other files.

I don't understand why you want that.

In most cases, the implicit cast is still there anyway. And casting does not enforce any alignment. In this example, without casting, you could have a crash if the alignment is not met. If you use this temporary variable and you cast, you will have the right alignment, but you could write data that you do not expect to write. So I'm not sure which is worst, but I think a crash would be better to catch issues here.

I would say it should not be changed in this PR that says to only change to const. Fixing this it is a different problem.

I agree that the current state is far from perfect. But random crash are not good either.
Having this line helped me debug a crash in someone else code with a buffer coming from the shell and there was no other mention that for this platform it should be aligned and having this in the code helped me find it.

I would prefer an API fix with a per platform type or alignment definition but I would say it should be another PR.

OK to re-add the temp variable but I would prefer keeping implicit cast, which is IMO more aligned with the rest of the code base.

I re-checked all the changes and my bad, I generalized this line as if you did the same to the other files.

For the other ones you just removed the cast part, which is true does not help at all. Keeping only the assignement to an uint32_t * variable is enough I agree. So you can keep them as you changed now as murdock does not issue warnings.

Here however you also removed the uint32_t *data_addr instead of only defining it const.
And from the MSC_WriteWordI implementation, it also cast the pointer to uint32_t somewhere, so I would prefer keeping this data_addr intermediate variable.

:D I was answering at the same time.

And yes, re-adding the temp variable here is enough for me.

OK, will do!

cladmi · 2018-03-14T15:29:17Z

I agree on the API change, not tested on mentioned hardware.
Lets see Murdock output.

kYc0o · 2018-03-14T15:45:41Z

Before merging, I was testing a bit more thoroughly this PR and discovered that the stm32 driver is not working properly, at least on my desk.

Can someone test on the F variants that test <page> works using tests/periph_flashpage ?

kYc0o

Sorry for this, I know it's not this PR the culprit but I'd like to have more testing on the flash driver for stm32f.

vincent-d · 2018-03-14T16:01:09Z

Works as expected for me om nucleo-f091 (stm32f091). What do you see @kYc0o ?

kYc0o · 2018-03-14T16:20:52Z

I'm on a nucleo-f072 (stm32f072rb) and when I do test 50 it hangs, without any error, it's just responseless... If I reset it still responseless (though I have serial output), but when I execute any command it doesn't answer. If I turn it off and on again I can again interact with it. I execute dump 50 and nothing is written. I have the latest st-link firmware. The same behaviour (even worse because it makes my board unflashable afterwards) on my IoT-Lab boards.

Very strange... Do you have access to IoT-Lab?

kYc0o · 2018-03-14T16:27:30Z

Ok so I just discovered that I have the right to write one command on each turn off/turn on round (another issue...). The pattern written by test 50 it's there and works.

kYc0o · 2018-03-14T16:39:38Z

However, I have no issues at all on nucleo-l152. It might be my board which has something strange. If we could have a valid test on iotlab-m3 it would be awesome. Yesterday I succeeded to test but today I have failed assertions with flashpage_write_raw.

vincent-d · 2018-03-14T16:54:47Z

I gave another try on nucleo32-f042 and it works like a charm.

I don't have access to IoT-Lab

kYc0o · 2018-03-14T17:05:05Z

This is my output on IoT-Lab

test 100
1521046351.786173;m3-100;> test 100

help
1521046356.899114;m3-100;error verifying the content of page 100: > help
1521046356.901197;m3-100;Command              Description
1521046356.903274;m3-100;---------------------------------------
1521046356.903466;m3-100;info                 Show information about pages
1521046356.903601;m3-100;dump                 Dump the selected page to STDOUT
1521046356.907255;m3-100;dump_local           Dump the local page buffer to STDOUT
1521046356.907447;m3-100;read                 Read and output the given page
1521046356.907583;m3-100;write                Write (ASCII) data to the given page
1521046356.909116;m3-100;write_raw            Write (ASCII, max 64B) data to the given address
1521046356.909308;m3-100;erase                Erase the given page
1521046356.912688;m3-100;edit                 Write bytes to the local page
1521046356.912881;m3-100;test                 Write and verify test pattern

Somehow the serial is retained... I don't know exactly what can be related but I'm investigating.

aabadie · 2018-03-14T17:12:28Z

Somehow the serial is retained... I don't know exactly what can be related but I'm investigating.

This is a bug in the test application a \n is missing at the end of error verifying the content of page 100:, just press enter. But this means that the values are not written correctly.

cladmi · 2018-03-14T17:15:34Z

It works on IoT-LAB with commits before #8768

cladmi · 2018-03-14T17:54:42Z

On iotlab-m3 it works when there is ENABLE_DEBUG in stm32_common/periph/flashpage.c both before and after this PR.
It also works on samr21-xpro

So this PR can be merged as the problem comes from somewhere else.

kYc0o · 2018-03-14T17:55:42Z

Ok, let's merge this then.

vincent-d added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation State: waiting for other PR State: The PR requires another PR to be merged first Area: drivers Area: Device drivers labels Mar 13, 2018

vincent-d requested review from aabadie and kYc0o March 13, 2018 15:55

vincent-d mentioned this pull request Mar 13, 2018

drivers: add mtd wrapper for periph_flashpage #8774

Merged

aabadie approved these changes Mar 13, 2018

View reviewed changes

drivers/flashpage: use const ptrs for write/verify

1f409d0

vincent-d force-pushed the pr/flashpage_const_ptr branch from f7c80f2 to 6c27e75 Compare March 13, 2018 20:52

vincent-d added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Mar 13, 2018

vincent-d requested a review from cladmi March 14, 2018 12:09

cladmi suggested changes Mar 14, 2018

View reviewed changes

cladmi added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Mar 14, 2018

cladmi approved these changes Mar 14, 2018

View reviewed changes

Vincent Dupont added 5 commits March 14, 2018 16:28

cpu/efm32: use const pointer in flashpage

4f44778

cpu/msp430: use const pointer in flashpage

5a2a4cf

cpu/nrf5x: use const pointer in flashpage

b402491

cpu/sam0: use const pointer in flashpage

a1f482c

cpu/stm32: use const pointer in flashpage

549bf0e

vincent-d force-pushed the pr/flashpage_const_ptr branch from 48f1edc to 549bf0e Compare March 14, 2018 15:28

kYc0o requested changes Mar 14, 2018

View reviewed changes

kYc0o approved these changes Mar 14, 2018

View reviewed changes

kYc0o merged commit 4f19001 into RIOT-OS:master Mar 14, 2018

toonst deleted the pr/flashpage_const_ptr branch August 21, 2018 15:31

Conversation

vincent-d commented Mar 13, 2018

Contribution description

Issues/PRs references

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Mar 13, 2018

Uh oh!

kYc0o commented Mar 13, 2018

Uh oh!

cladmi commented Mar 14, 2018

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Mar 14, 2018

Uh oh!

kYc0o commented Mar 14, 2018

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Mar 14, 2018

Uh oh!

kYc0o commented Mar 14, 2018

Uh oh!

kYc0o commented Mar 14, 2018

Uh oh!

kYc0o commented Mar 14, 2018

Uh oh!

vincent-d commented Mar 14, 2018

Uh oh!

kYc0o commented Mar 14, 2018

Uh oh!

aabadie commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cladmi commented Mar 14, 2018

Uh oh!

cladmi commented Mar 14, 2018

Uh oh!

kYc0o commented Mar 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cladmi Mar 14, 2018 •

edited

Loading

aabadie commented Mar 14, 2018 •

edited

Loading