Thanks for the response, Will. I made it one-byte because the scenario where the CRC check strength comes into play is somewhat rare; it is to detect partial writes. I.e. when fcb_append_finish() fails to get called. This, presumably, would onlly happen on crash/power outage within a specific time window. This is not used as an error detection mechanism on a channel where we expect bit errors.
The way I did it was I added 2 syscfg knobs to control which CRC is included in the build. In case you get really tight on code space. Of course, newtmgr uses CRC16, so if you have that enabled, there is no gain. There’s 3 different options when starting FCB: inherit from flash, force 16 bit, or force 8 bits. If the flash region has not been initialized with anything, then the ‘inherit’ option will prefer 16 bits over 8 bits. So if you need to worry about backwards compatibility, set ‘inherit’. If you want to use 16 bit, and are flashing a new device, use ‘inherit’ or set ‘force 16bits’. If you need to keep FCB region backwards compatible, use option ‘force 8 bits’. > On Jul 27, 2018, at 11:05 PM, will sanfilippo <wi...@runtime.io> wrote: > > I realize I am a bit late with these comments but better late than never I > guess. Well, maybe some or all of these comments should never be made :-) > This could be a horrible idea but why not just make it backward incompatible? > (for #2). Maybe have a syscfg for folks that want to use the old format in > the code? I would even consider making all entries have a two-byte CRC > (regardless of length of data written). I presume the resume that it was > one-byte was to save flash space? Heck, maybe even allow the syscfg to > specify a 32-bit CRC (for those with lots of flash and/or really do not want > a false positive to occur). > > Will >> On Jul 26, 2018, at 5:10 AM, marko kiiskila <ma...@runtime.io> wrote: >> >> Hi, >> >> there’s few issues with FCB which I need to fix. >> >> 1) Compressing flash sector in the middle of write. >> Currently the cycle of operation is expected to be the following: >> fcb_append() -> get offset to write data to >> flash_area_write() -> write data >> fcb_append_finish() -> writes CRC etc >> >> This is not too great if the location returned by fcb_append() gets rotated >> to be >> scratch area before fcb_append_finish() is called. Which is quite possible >> if you’re using FCB to store logs, and data comes from different tasks. >> >> 2) CRC is 1 byte only. It’s ok for short config entries, but for longer >> things >> (log entries) is not good enough. >> >> To fix 1 I was going to add linkage to fcb_entry such that FCB can track >> of incomplete writes, and add a write routine which should be used when >> doing the write. >> >> Issue 2 is a little bit more cumbersome. I want to be able to at least >> allow backwards compatibility, where code figures out from data in >> the flash which CRC format is there. But then be able to switch to >> 16 bit CRC format as new flash areas are written. Or keep using >> the old format, because you want to do downgrade to old SW version >> if necessary. >> >> So that’s what I’m currently thinking of trying to achieve. Let me know >> if you guys have other things to take into consideration here. >
