[ext] Adding ext json library and example

[hshose]

Added the nlohmann json library, actually the single header include version.

I more or less copied what other ext modules do with their update script and all. Feel free to move this into the modm-ext github namespace.

Example creates and retrieves data from plain JSON and BSON written to FLASH, see #1145.

Probably heaps of room for improvement in the example, I wasn't really sure if I should use the modm::accessor::Flash since it doesn't do anything on the G4 as far as I could see?

[salkinium]

Very nice!

I've created https://github.com/modm-ext/json-partial and made you a maintainer. Please push your repository there, add a smol README and make sure the GH actions keeps the repo up-to-date (see this update.yml for an example). Please move the submodule to ext/nlohmann/json and put the lbuild file in ext/nlohmann/module.lb. This aligns it with the other submodules. (We even have a secret guide for adding external code like this :-)

[chris-durand]

Nice!

I wasn't really sure if I should use the modm::accessor::Flash

modm::accessor::Flash is only relevant for AVR where flash and RAM don't share the same address space.

[chris-durand]

That flash API is really messy to work with... It would really be nice to also have overloads that take pointers for memory locations instead of integers. Even void* would be a big type safety improvement over uintptr_t because we could get rid of all the reinterpret_casts and mixing up address and data would be a hard compile error instead of a silent runtime one.

If we had a memcpy like overload of Flash::program the user wouldn't need to write that verbose loop with all the casts. I'll look into it tomorrow. Coincidentally, I had to write similar code at work today storing data in flash on a STM32F1 and wasn't very pleased with the API.

Also *(Flash::MaxWordType*)src_addr is undefined behavior in both C and C++ if src_addr doesn't point to an actual object of type MaxWordType and MaxWordType is neither of char, unsigned char or std::byte.

The usual conforming way to write this is using a memcpy that is optimized away by the compiler. Code generation should be equivalent with at least -Og if the hardware can do the access. In this particular case with a MaxWordType of uint64_t it can't and dereferencing the pointer will crash on ARMv7M for not 64-bit aligned source data, if I'm not mistaken. From a language point of view this is always UB independent of alignment.

[hshose]

I think I blindly copied the flash access part from one of the flash examples .

Even void* would be a big type safety improvement over uintptr_t because we could get rid of all the reinterpret_casts and mixing up address and data would be a hard compile error instead of a silent runtime one.

Yes, happened to me quite a few times while I tried figuring out how the flash works.

[hshose]

Ok, I've changed the loop to memcpy; is that what you had in mind?
This is still not fixing the problem with the flash::program api imho but definitely looks nicer than what I had before.

const auto flash_write_base_addr{reinterpret_cast<uint32_t>(Flash::getAddr(page_start))};
for (size_t ii = 0; ii < alice_binary.size(); ii += sizeof(Flash::MaxWordType))
{
	Flash::MaxWordType outdata;
	memcpy(&outdata, &alice_binary[ii], sizeof(Flash::MaxWordType));
	err |= Flash::program(flash_write_base_addr+ii, outdata);
}

[hshose]

Another thing, that @rleh brought up yesterday and @chris-durand originally mentioned in his reply to #1145 (if I understood correctly, please correct me if not): Would it be reasonable to integrate a "persistent" flash section at the end of flash in the linkerscript (size configured with lbuild option)?

For my STM32G474 example, resulting in something like:

MEMORY
{
	FLASH (rx) : ORIGIN = 0x08000000, LENGTH = 522241
	PERSISTENT_FLASH (rx) : ORIGIN = 0x807f801, LENGTH = 2047
        /*... etc ... */
}

__flash_start = ORIGIN(FLASH);
__flash_end = ORIGIN(FLASH) + LENGTH(FLASH);
__persistent_flash_start = ORIGIN(PERSISTENT_FLASH);
__persistent_flash_end = ORIGIN(PERSISTENT_FLASH) + LENGTH(PERSISTENT_FLASH);
/*... etc ... */

I would probably do this based on an lbuild option;
Something like a :platform:cortex-m:persistent_flash_size that defaults to zero.

Would this be done inside modm-devices or the cortex-m linkerscript?

E.g., here in the linkerscript add something like

%% for memory in memories
%% if memory.name == "FLASH" and options[":platform:cortex-m:persistent_flash_size"] is 0
{{ memory.name | upper }} ({{ memory.access }}) : ORIGIN = {{ "0x%08X" % memory.start }}, LENGTH = {{ memory.size - options[":platform:cortex-m:persistent_flash_size"] }}
PERSISTENT_FLASH (rx) : ORIGIN = {{ "0x%08X" % (memory.start + memory.size - options[":platform:cortex-m:persistent_flash_size"])}}, LENGTH = {{options[":platform:cortex-m:persistent_flash_size"]}}
%% else
{{ memory.name | upper }} ({{ memory.access }}) : ORIGIN = {{ "0x%08X" % memory.start }}, LENGTH = {{ memory.size }}
%% endif
%% endfor

and similarly here to get the __persistent_flash_start and __persistent_flash_end.

This is my first time touchign a modm linkerscript and its a bit intimidating.

Also not sure, if this is even a reasonable feature for modm to have?

[salkinium]

Also not sure, if this is even a reasonable feature for modm to have?

Yes, we already have an offset of the FLASH start address in the linkerscript to accommodate bootloaders:

modm/src/modm/platform/core/cortex/module.lb

Lines 232 to 239 in c43be03

	flash_size = next((int(x['size']) for x in memories if x['name'] == 'flash'), 16*1024*1024)
	module.add_option(
	NumericOption(
	name="linkerscript.flash_offset",
	description=FileReader("option/flash_offset.md"),
	minimum=0,
	maximum=hex(flash_size),
	default=0))

Your application is basically the same, but from the other side. (You cannot use the front, since that's where the vector table needs to be placed). Remember to validate the option so that it doesn't collide with the flash offset option:

modm/src/modm/platform/core/cortex/module.lb

Lines 279 to 294 in c43be03

	def validate(env):
	flash_offset_option_name = "modm:platform:cortex-m:linkerscript.flash_offset"
	flash_offset = env[flash_offset_option_name]
	if flash_offset != 0:
	# The offset needs to be aligned regarding the number of interrupts
	# in order for the vector table relocation to work. Every interrupt
	# requires 4 bytes, and therefore two additional bits of alignment.
	number_of_interrupts = env.query(":::vector_table")["highest_irq"] + 16
	bit_alignment = number_of_interrupts.bit_length() + 2
	mask = (1 << bit_alignment) - 1
	if flash_offset & mask:
	raise ValidateException("Invalid flash offset in option '{}' (value=0x{:X}). "
	"The offset needs to be {} bit aligned."
	.format(flash_offset_option_name,
	flash_offset,
	bit_alignment))

I'd call it :platform:cortex-m:linkerscript.flash_reserved analog to :platform:cortex-m:linkerscript.flash_offset.

[hshose]

Thanks @salkinium for the advice! This was really helpful.

I have implemented the flash_reserved section and some validation (that flash_reserved is not bigger than flash_size - flash_offset).

Seems to work in the example.

[salkinium]

Thanks, I will review this weekend in depth.

[hshose]

I'm not super happy with this, because it doesn't adhere to the category 3 flash page numbering in RM0440, which goes from 0...127 on both banks.
I think the Flash::erase function should actually have a second parameter where the user can specify the bank.
However, that would break the Flash::getPage function and potentially other things I don't want to fix right now.

Also, the flash page numbering on the g0 devices is even more crude (bank 1 goes from 0...128 or so) and bank 2 goes from 256... on. 🧠

Does anyone have a better idea how to do this?

[salkinium]

The flash page size differs depending on the dual bank option byte? Ie. if dual bank, it's 2048B, otherwise it's 4096B??? Then we need to modify shift to be (FLASH->OPTR & FLASH_OPTR_DBANK) ? 11 : 12) in getPage(), getOffset() and getSize(), which is ugly, but would solve the problem?

[hshose]

Yes :-D looks like thats the case!

[hshose]

I think the CI fails from your changes are due to the FLASH_CR_BKER and FLASH_OPTR_DBANK only beeing available on G4 devices with category 3 flash. On G4 devices with category 2 and 4 flash, there is no dual bank option 🧠

[salkinium]

Ok, I implemented it that way. Apparently only G47x and G48x are category 3 devices, so let's see if I fixed it.

[salkinium]

And G0Bx and G0Cx are the only dual-bank G0 devices.

[salkinium]

I've polished the changes a litte:

• Added a GitHub Actions workflow to the json-partial repo and a Readme.
• moved the submodule to nlohmann/json to be in line with the other submodules.
• Mild refactoring and reformulation of the flash_reserved code.
• Refactored the dual flash bank code to use a runtime check of the option bytes.
• Squashed and the commits.

Please test this on real hardware!
(I reverted your previous commits so that you don't loose the code due to my force push)

[hshose]

Thank you @salkinium for all the fixes and help with this!
I'll test more on hardware tomorrow; I have G474 and G431 devboards here and if I remember correctly there, should also be someⓇ G0 devboard somewhere™.

[salkinium]

FYI: No pressure, but if you want this in the 2024q1 release, it needs to be merged by monday evening.

[salkinium]

(Squashed and rebased.)

[hshose]

Yeah sorry this didn't happen until last night unfortunately.

I have tested on G474 and G431 now and it works on both boards. I coulnd't find a G0 board to test with 😢

I also took the liberty to rename the example from json to flash_json, because half the example is actually about using flash not about using the JSON library.

[rleh]

Don't we need to do +1 to get the first page completely inside the reserved flash area?

[hshose]

I think the reserved size in this case is a multiple of the flash page size (1024*16 in the project.xml of the example) so the reserved start should align with a flash page?

[hshose]

I added an exception in the lbuild module to only allow multiples of 8Kbytes reserved flash (8Kbytes was the largest flash page size I found for some H7 devices). Now reserved flash should always start on its own page. Is this what you had in mind?

[hshose]

@rleh ?

[salkinium]

I lost track, is this ready to merge @rleh?

[salkinium]

I'll merge this weekend (minus the last commit).

[salkinium]

STM32 has different sizes for flash pages, so this check is not scalable. There might also be use-cases that would warrant non-aligned reservation sizes (like merging two binaries into one).

[hshose]

@rleh had a strong opinion about this one so that's why I added it.
I myself think that it should be user responsibility to only reserve multiples of the given flash page size and/or take other precautions to not erase program data otherwise, so feel free to drop the last commit.

Regarding STM32's different flash sizes: the 8Kbytes should be an integer multiple of all possible STM32 flash page sizes, thus work for all models (if you want to be extra sure, maybe 16Kbytes are a safer option).

As of intentionally wanting non-aligned reserved sizes, @rleh was of the opinion that this would be rare and/or user responsibility to hack that together.

[salkinium]

I think this should be the responsibility of a flash storage module that then probably also has other restrictions like a minimum number of flash pages. For example, littlefs requires at least a few flash pages to work correctly, so this requirement is already too weak. Let's add this later once we have a concrete use case.

I also recently discovered there is a database inside the STM32CubeProgrammer with all the memory sizes and flash pages, so we could generate the flash size code in the :platform:flash module and use it for queries like this.

[salkinium]

macOS CI is broken again, I'm so tired of that crap.

[hshose]

Thanks for merging!!! ❤️ 💘