sys/flash_utils: helpers to store data in flash

[maribu]

Contribution description

This helpers that allow storing, accessing, and working with data in flash that works for both classical Harvard architectures (which do not map flash also into the address space) as well as modern Harvard architectures and von-Neumann architectures.

With this, examples/default again runs on the Arduino Uno / Nano. Since this board is still the "entry kit" for many people to embedded hardware, it would be nice to support it with our default example.

Testing procedure

examples/default should run and work on ATmega boards (especially ATmega328P and ATmega32U4 based boards) as well on all other boards now.

Issues/PRs references

None

[maribu]

I think this is ready for review now. I could split out the actual users into separate PRs, though.

_flash et all, where do we use that?

Exactly where it is introduced now:

1. shell command names and their help strings
   • this is fully transparent and only people actually working on the shell interpreter will notice
2. unit string (phydat) and sensor / actuator strings (SAUL)
   • this is mostly transparent, as except for the shell command I expect few users of those strings
3. In ps
   • this doesn't safe as much RAM as the other two, but it is fully transparent and ps.c is as "finished" as code can be

Right now we loose the AVR platform on these three items. This PR as is wins the platform back and makes the main features usable again on the most common board. There will be a few bytes here and there that could also be safed, but this really was the major chunk that actually makes a difference.

the mapped functions (strlen, strcmp, ...) are difficult enough to get right with the combination of multiple libcs, multiple compilers, multiple intrinsics (libgcc, compiler-rt), multiple optimization levels. Adding another layer on top is opening a can of worms.

We have sys/fmt which is more pain for less gain.

Also, this doesn't add any new functions. Right now the way to go is:

#ifdef __AVR__
    puts_P(PSTR("Hello, world!"));
#else
    puts("Hello, world!");
#endif

With this PR users can simplify their code to:

    flash_puts(TO_FLASH("Hello, world!"));

It is just about making the dispatching between the printf() functions easier.

macros. they seem to creep in lately. but they suck.

Note that the macros as simple as

#define flash_printf printf_P

or

#define flash_printf printf

That certainly is better than

#ifdef __AVR__
    puts_P(PSTR("Hello, world!"));
#else
    puts("Hello, world!");
#endif

But back to the main point: Those old shitty ATmega Arduino boards are still the first board people buy when they get in touch with microcontroller development. We have actually quite a lot of PRs from "outside contributors", e.g.

• https://github.com/RIOT-OS/RIOT/pull/15494
• https://github.com/RIOT-OS/RIOT/pull/15712
• https://github.com/RIOT-OS/RIOT/pull/15758
• https://github.com/RIOT-OS/RIOT/pull/15998
• https://github.com/RIOT-OS/RIOT/pull/16288
• https://github.com/RIOT-OS/RIOT/pull/16587
• https://github.com/RIOT-OS/RIOT/pull/17387
• https://github.com/RIOT-OS/RIOT/pull/18245

If we could keep them motivated to stick with RIOT and contribute more, that would be immensely helpful. But right now the default example greets them with a linking error. We can fix that.

[kfessel]

forcing program strings into flash seem like good idea for a embedded OS, sadly __flash won't work with C++. __flash seems modern and outdated at the same time, C++ will not support this (not even in extern(C))

[maribu]

OK, so flash_utils is C only. This means it may not be included in any non-internal header. But this is not much of an issue, as this should be contained well anyway.

I have to rework the SAUL / phydat changes, though.

[maribu]

This is now self-contained also for the <foo>_to_str() functions in SAUL and phydat, so that C++ code never will indirectly include flash_utils.h. However, this deprecates the phydat_unit_to_str() and saul_class_to_str(), as those use a shared in-RAM buffer for AVR that doesn't work well with concurrent use or even printf("%s, %s", phydat_unit_to_str(UNIT_NONE), phydat_unit_to_str(UNIT_UNDEF));. Hence the API change label.

However, users will want to either print those strings (that is now covered) or write that string into some buffer for sending it out via network (that is now also covered) anyway. And non-AVR users actually can keep using the function without fear or overhead.

[benpicco]

IIRC you mentioned you had an idea on how to get rid of the TO_FLASH in flash_printf(TO_FLASH()) - that would make this a lot more palatable

[maribu]

IIRC you mentioned you had an idea on how to get rid of the TO_FLASH in flash_printf(TO_FLASH()) - that would make this a lot more palatable

I tried, it didn't work. The idea was to use _Generic() to issue the TO_FLASH() instead. The first issue is that _Generic() cannot tell string literals (which are the only type compatible with TO_FLASH()) apart from char * strings actually allocated in RAM. Here, the proposed solution was to just force with coccinelle that the first parameter of printf() always a string literal or __flash const char *. I didn't really solve this yet, as I have to dive a bit deeper into the coccinelle doc to understand how this could be expressed as semantic patch.

The second issue however is a blocking one: The TO_FLASH() macro really only works at a preprocessor level. But _Generic() works at the C language level. So there is absolutely no way to do this with _Generic(). I am almost certain that with even the most recent C standard, there is no way to get rid of TO_FLASH().

One could provide a printf_literal(x, ...) that would be an alias for flash_printf(TO_FLASH(x), ...). But I think that this would actually increase the complexity without being a significant gain in readability.

[benpicco]

One could provide a printf_literal(x, ...) that would be an alias for flash_printf(TO_FLASH(x), ...). But I think that this would actually increase the complexity without being a significant gain in readability.

Hm you think so? I feel like most printf invocations will have the format string as string literal, the cases where the format string is a variable (but still const on flash) seem to be so rare that I would even not care about them at all.

Having that macro in every printf statement on the other hand is very ugly.

[riot-ci]

Murdock results

:heavy_check_mark: PASSED

7e58bea1bd13b364d179be6a1ae1f655c41ca877 examples,tests: Update Makefile.cis

Success	Failures	Total	Runtime
6864	0	6864	08m:01s

Artifacts

• Documentation preview

[benpicco]

Ok, CI still has a different opinion on this

[maribu]

Ok, CI still has a different opinion on this

The AVR libc has a define for the macro AES_KEY:

$ rg AES_KEY /usr/avr/include
[...]
/usr/avr/include/avr/iom256rfr2.h
4543:#define AES_KEY                         _SFR_MEM8(0x13F)
[...]

The AVR stdio wrapper needs to include <avr/pgmspace.h> on AVR to handle the shifting to progmem, but that in turn includes <avr/io.h> which for some AVR devices provides an AES_KEY macro :/

[benpicco]

I don't get why we have

typedef struct aes_key_st {
    /** @cond INTERNAL */
    uint32_t rd_key[4 * (AES_MAXNR + 1)];
    int rounds;
    /** @endcond */
} AES_KEY;

RIOT coding convention would be aes_key_t

[maribu]

Agreed! But I'd rather not change this in this PR :)

[maribu]

some missing letters ?

Good catch, thx!

[maribu]

I squashed and dropped the changes to fix the naming clash with AES_KEY, as https://github.com/RIOT-OS/RIOT/pull/19290 is fixing that better and on its way in. Once that is merged, this PR hopefully passes the CI.

[maribu]

@kaspar030: I think all your concerns have been addressed. Care to take another look?

[maribu]

@fabian18: Good catch, thx :)

[maribu]

waiting for

• [ ] https://github.com/RIOT-OS/RIOT/pull/19292

[maribu]

I updated the phydat code to no longer fix the unit symbol for gram, so it no longer depends on fixing the phydat unit.

Fixing phydat units seemed to be a quick fix, but turned into a larger endeavor. So I'd rather not have this PR depending on fixing a unit symbol.

[benpicco]

Fixing phydat units seemed to be a quick fix, but turned into a larger endeavor.

eh, you could have the ACK (and passing CI) by just adding two compat defines :wink:

[kaspar030]

@kaspar030: I think all your concerns have been addressed. Care to take another look?

I don't think they have. We're still changing how to store things in flash, for which on all but avr, a simple "const" is enough. Now, not anymore. We'll carry this extra burden for all architectures, forever.

I don't think that we should have these architecture specific hacks throughout the code base.

Limiting this to saul, phydat, and shell might be viable. Can we maybe discuss this at the VMA? I'll add it as topic.

[maribu]

bors try

[benpicco]

bors try

[bors[bot]]

try

Already running a review

[benpicco]

bors cancel bors try

[bors[bot]]

try

Already running a review

[maribu]

bors cancel bors try

[bors[bot]]

try

Already running a review

[maribu]

bors cancel

[maribu]

bors try

[bors[bot]]

try

Already running a review