Hi Marko and Will, I have been studying fcb and I think you can leave it at 8 bit as it was. The crc can only be interpreted as a check that the closing was done properly. As soon as the crc check fails this only means that the closing failed. It could just as well be fixed to zero instead of a crc.
When writing errors (bit errors) would occur fcb would need a lot more protection, the length which is written first could no longer be trusted and it would be impossible to find the crc. The writing of the length can also be a more problematic case to solve, what happens when the write of the length fails and the length that is read is pointing outside the sector ? Kind regards, Jehudi Op ma 30 jul. 2018 om 11:10 schreef marko kiiskila <ma...@runtime.io>: > > 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. > > >
