[ext] Adding ext json library and example #1146

hshose · 2024-03-12T16:48:01Z

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 · 2024-03-12T21:54:55Z

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 · 2024-03-12T22:22:31Z

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 · 2024-03-12T23:27:05Z

examples/nucleo_g474re/json/main.cpp

+		for (uint32_t src_addr = reinterpret_cast<uint32_t>(&alice_binary[0]),
+					  dst_addr{reinterpret_cast<uint32_t>(Flash::getAddr(page_start))};
+			 src_addr < (reinterpret_cast<uint32_t>(&alice_binary[0]) + alice_binary.size());
+			 src_addr += sizeof(Flash::MaxWordType), dst_addr += sizeof(Flash::MaxWordType))
+		{
+			err |= Flash::program(dst_addr, *(Flash::MaxWordType*)src_addr);
+		}


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.

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.

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 · 2024-03-13T20:13:25Z

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 · 2024-03-17T13:21:31Z

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 · 2024-03-20T13:41:01Z

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 · 2024-03-20T19:09:02Z

Thanks, I will review this weekend in depth.

hshose · 2024-03-20T20:43:12Z

src/modm/platform/flash/stm32/flash.cpp.in

+%% if dual_bank
+%% if family == "g0"
+	const bool index_is_on_second_bank{index >= 256};
+%% else
+	const bool index_is_on_second_bank{index >= (Size/2048/2)};
+%% endif
+	const uint8_t page = index_is_on_second_bank ? index - Size/2048/2 : index;
+	FLASH->CR = FLASH_CR_STRT | FLASH_CR_PER |
+			((index_is_on_second_bank << FLASH_CR_BKER_Pos) & FLASH_CR_BKER_Msk) |
+			((page << FLASH_CR_PNB_Pos) & FLASH_CR_PNB_Msk);
+%% else


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?

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?

Yes :-D looks like thats the case!

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 🧠

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

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 · 2024-03-27T22:03:16Z

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 · 2024-03-30T14:04:35Z

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

salkinium · 2024-03-30T15:08:34Z

(Squashed and rebased.)

hshose · 2024-04-02T09:09:23Z

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 · 2024-04-02T13:42:12Z

examples/nucleo_g474re/flash_json/main.cpp

+
+		// put BSON in reserved flash
+		uint32_t err{0};
+		const size_t page_start =


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

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?

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?

examples/nucleo_g474re/flash_json/main.cpp

salkinium · 2024-04-10T22:34:23Z

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

salkinium

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

salkinium · 2024-05-17T08:52:32Z

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

+                                    "is not a multiple of 8Kbytes, which could "
+                                    " lead to accidential program flash erasure."
+                                    .format(flash_reserved_option_name,
+                                            flash_reserved))


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).

@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.

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.

Co-authored-by: Niklas Hauser <niklas.hauser@rwth-aachen.de>

salkinium · 2024-05-19T10:40:40Z

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

hshose · 2024-05-20T13:22:23Z

Thanks for merging!!! ❤️ 💘

hshose force-pushed the feature/ext-nlohmann-json branch 2 times, most recently from 2bb9fe2 to b8d5a8d Compare March 12, 2024 18:41

salkinium added advanced 🤯 feature 🚧 example 🔑 labels Mar 12, 2024

salkinium added this to the 2024q1 milestone Mar 12, 2024

chris-durand reviewed Mar 12, 2024

View reviewed changes

hshose force-pushed the feature/ext-nlohmann-json branch from 7a1fb67 to 4f0c155 Compare March 13, 2024 20:33

hshose force-pushed the feature/ext-nlohmann-json branch from 4f0c155 to 9017d04 Compare March 20, 2024 13:39

hshose force-pushed the feature/ext-nlohmann-json branch 2 times, most recently from 736dbc8 to 77a2df1 Compare March 20, 2024 15:01

salkinium self-requested a review March 20, 2024 19:08

hshose mentioned this pull request Mar 20, 2024

[STM32G4] flash dual bank ignored during erase #1151

Closed

hshose force-pushed the feature/ext-nlohmann-json branch from b543242 to 7eabed0 Compare March 20, 2024 20:37

hshose commented Mar 20, 2024

View reviewed changes

hshose force-pushed the feature/ext-nlohmann-json branch 2 times, most recently from 2fea03d to 6ff163d Compare March 20, 2024 23:41

salkinium approved these changes Mar 25, 2024

View reviewed changes

salkinium added the ci:hal Triggers the exhaustive HAL compile CI jobs label Mar 27, 2024

salkinium force-pushed the feature/ext-nlohmann-json branch from de6a0c2 to 1337665 Compare March 30, 2024 15:07

salkinium force-pushed the feature/ext-nlohmann-json branch from 1337665 to 088a1e0 Compare March 30, 2024 15:12

rleh modified the milestones: 2024q1, 2024q2 Apr 1, 2024

hshose force-pushed the feature/ext-nlohmann-json branch 2 times, most recently from c317b81 to be1e677 Compare April 2, 2024 09:07

rleh approved these changes Apr 2, 2024

View reviewed changes

rleh removed the ci:hal Triggers the exhaustive HAL compile CI jobs label Apr 7, 2024

hshose force-pushed the feature/ext-nlohmann-json branch from be1e677 to 1142c5c Compare April 10, 2024 11:27

salkinium requested a review from rleh April 22, 2024 07:44

hshose mentioned this pull request May 3, 2024

Linker issues with newlib version 4.4.0 #1160

Open

salkinium approved these changes May 17, 2024

View reviewed changes

hshose and others added 4 commits May 19, 2024 11:12

[cortex-m] added flash_reserved section

f0e5a2a

Co-authored-by: Niklas Hauser <niklas.hauser@rwth-aachen.de>

[ext] added nlohmann json submodule

cde35ed

[ext] example for json usage added

4c79529

[flash] STM32G4 flash erase dual bank BKER bit

71becdc

Co-authored-by: Niklas Hauser <niklas.hauser@rwth-aachen.de>

salkinium force-pushed the feature/ext-nlohmann-json branch from 0e819e9 to 71becdc Compare May 19, 2024 09:13

salkinium merged commit 71becdc into modm-io:develop May 19, 2024
11 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ext] Adding ext json library and example #1146

[ext] Adding ext json library and example #1146

hshose commented Mar 12, 2024

salkinium commented Mar 12, 2024

chris-durand commented Mar 12, 2024

chris-durand Mar 12, 2024 •

edited

Loading

hshose Mar 13, 2024

hshose Mar 20, 2024

hshose commented Mar 13, 2024

salkinium commented Mar 17, 2024

hshose commented Mar 20, 2024 •

edited

Loading

salkinium commented Mar 20, 2024

hshose Mar 20, 2024

salkinium Mar 25, 2024

hshose Mar 27, 2024

hshose Mar 27, 2024

salkinium Mar 27, 2024

salkinium Mar 27, 2024

salkinium left a comment

hshose commented Mar 27, 2024

salkinium commented Mar 30, 2024

salkinium commented Mar 30, 2024

hshose commented Apr 2, 2024

rleh Apr 2, 2024

hshose Apr 10, 2024 •

edited

Loading

hshose Apr 29, 2024

hshose May 1, 2024

salkinium commented Apr 10, 2024

salkinium left a comment

salkinium May 17, 2024

hshose May 17, 2024

salkinium May 17, 2024

salkinium commented May 19, 2024

hshose commented May 20, 2024

[ext] Adding ext json library and example #1146

[ext] Adding ext json library and example #1146

Conversation

hshose commented Mar 12, 2024

salkinium commented Mar 12, 2024

chris-durand commented Mar 12, 2024

chris-durand Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hshose commented Mar 13, 2024

salkinium commented Mar 17, 2024

hshose commented Mar 20, 2024 • edited Loading

salkinium commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salkinium left a comment

Choose a reason for hiding this comment

hshose commented Mar 27, 2024

salkinium commented Mar 30, 2024

salkinium commented Mar 30, 2024

hshose commented Apr 2, 2024

Choose a reason for hiding this comment

hshose Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salkinium commented Apr 10, 2024

salkinium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salkinium commented May 19, 2024

hshose commented May 20, 2024

chris-durand Mar 12, 2024 •

edited

Loading

hshose commented Mar 20, 2024 •

edited

Loading

hshose Apr 10, 2024 •

edited

Loading