-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ext] Adding ext json library and example #1146
Conversation
2bb9fe2
to
b8d5a8d
Compare
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 |
Nice!
|
examples/nucleo_g474re/json/main.cpp
Outdated
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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_cast
s 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
}
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 For my STM32G474 example, resulting in something like:
I would probably do this based on an Would this be done inside modm-devices or the cortex-m linkerscript? E.g., here in the linkerscript add something like
and similarly here to get the 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? |
7a1fb67
to
4f0c155
Compare
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
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
I'd call it |
4f0c155
to
9017d04
Compare
Thanks @salkinium for the advice! This was really helpful. I have implemented the Seems to work in the example. |
736dbc8
to
77a2df1
Compare
Thanks, I will review this weekend in depth. |
b543242
to
7eabed0
Compare
%% 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes :-D looks like thats the case!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🧠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I implemented it that way. Apparently only G47x and G48x are category 3 devices, so let's see if I fixed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And G0Bx and G0Cx are the only dual-bank G0 devices.
2fea03d
to
6ff163d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Thank you @salkinium for all the fixes and help with this! |
FYI: No pressure, but if you want this in the 2024q1 release, it needs to be merged by monday evening. |
de6a0c2
to
1337665
Compare
(Squashed and rebased.) |
1337665
to
088a1e0
Compare
c317b81
to
be1e677
Compare
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 |
|
||
// put BSON in reserved flash | ||
uint32_t err{0}; | ||
const size_t page_start = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to do +1
to get the first page completely inside the reserved flash area?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rleh ?
be1e677
to
1142c5c
Compare
I lost track, is this ready to merge @rleh? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge this weekend (minus the last commit).
"is not a multiple of 8Kbytes, which could " | ||
" lead to accidential program flash erasure." | ||
.format(flash_reserved_option_name, | ||
flash_reserved)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Co-authored-by: Niklas Hauser <niklas.hauser@rwth-aachen.de>
0e819e9
to
71becdc
Compare
macOS CI is broken again, I'm so tired of that crap. |
Thanks for merging!!! ❤️ 💘 |
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?