-
Notifications
You must be signed in to change notification settings - Fork 2.1k
sam0 flashpage_write issue #7667
Description
This took a while to chase this down. On the samd21 issue, the flash page_write function casts the void* data argument to a uint32_t* pointer. This works fine when the compiler places the memory for your argument aligned. But I've found that it doesn't always. I was declaring a
static u8 Row[NVMCTRL_ROW_SIZE];
The compiler was alternately placing this memory at x20002402 or x20002400. At the latter, the cast to a uint32_t* was fine, but at the former, chip level exception occured. This kind of bug is no fun to figure out.
I have three proposals I'd like to consider for a fix. With some direction, I'll work up the appropriate PR.
-
Require a client side fix. In this case, we put the onus of getting the
dataargument 32bit aligned. One can use an expression like:static __attribute__((aligned(4))) u8 Row[NVMCTRL_ROW_SIZE];
to force the alignment. If this is the preferred solution, then the change that I believe should be made is the documentation of flashpage_write() should be modified to make this clear to those who decide to use this function.
This is not my personally preferred solution.
-
Use
uint16_tinstead ofuint32_t. I think it's OK to assume that memory is 2 byte aligned (see Interrupt-Handling on msp430 is buggy #3 otherwise).page_addranddata_addrbecomeuint16_t*and the loop looks like:for (unsigned i = 0; i < (FLASHPAGE_SIZE / 2); i++) { *page_addr++ = data_addr[i]; } -
There is a pathological case where someone does not have the argument short aligned, e.g.:
#pragma pack(1)
struct foo {
u8 singleByte;
u8 data[FLASHPAGE_SIZE];
};
#pragma pack(8)
In this case, it's again not safe to cast data as a uint16_t*. So data_addr must become a uint8_t* (page_addr remains uint16_t*). And the loop must do the work directly:
for (unsigned i = 0; i < (FLASHPAGE_SIZE - 1); i+= 2) {
uint16_t pair = ((uint16_t)data_addr[i + 1] << 8) | (uint16_t)(data_addr[i]);
*page_addr++ = pair;
}
This is my preferred solution, I think. It's the most robust and safe. It also matches the ASF implementation most closely.
(aside: I had hoped to use memcpy or wmemcpy, since they often dynamically deal with aligned/unaligned data nicely, but I could not get them to work--the data_addr is "special" enough to require explicit code apparently)