Hi, > On Jul 30, 2018, at 1:47 PM, Laczen JMS <[email protected]> wrote: > > 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.
That is a valid point. If you can’t trust your data path to flash, what can you trust? > > 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 ? On the other hand, there are flashes where multiple write cycles to same location are allowed; you can keep turning more bits to zero. There you can corrupt the data after writing. And on some the default, unwritten state is 0 (this we need to address for FCB, image management). You’re right; adding mechanisms to detect corruption of length field, for example, starts to get complicated and costly. Recovery is easier to do, however, if we use a stronger version of CRC. I.e. if CRC does not match, then just go forward a byte at a time until it does. > > Kind regards, > > Jehudi > > > Op ma 30 jul. 2018 om 11:10 schreef marko kiiskila <[email protected]>: >> >> 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 <[email protected]> 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 <[email protected]> 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. >>> >>
