[platform] H7 Flash now uses interrupts

[jgmess-dlr]

No description provided.

[sylvesterkaczmarek]

There is an off-by-one write here: bank is 1 or 2, while bankStatus has indices 0 and 1. Bank 1 updates the wrong slot and bank 2 writes past the array. This should use bankStatus[bank - 1].

[sylvesterkaczmarek]

Hi, one API-flow issue I noticed: unlock() used to call Flash::enable(), but that call is removed just as enable() gains the important NVIC_EnableIRQ(FLASH_IRQn) behaviour. Existing callers that only unlock before erase/program will therefore never enable the interrupt and can leave the new async status stuck. Could the IRQ enable stay in the existing initialization path?

[sylvesterkaczmarek]

Hi, the new failure state looks unreachable for flash errors that do not also generate EOP. initiateErase/Program() only enables EOPIE, while H7 exposes separate error interrupt enables such as WRPERRIE, PGSERRIE, STRBERRIE, INCERRIE, and OPERRIE. A failed operation can therefore leave bankStatus stuck at operationOngoing; could the error interrupts be enabled and handled independently of EOP?

[jgmess-dlr]

Hi, the new failure state looks unreachable for flash errors that do not also generate EOP. initiateErase/Program() only enables EOPIE, while H7 exposes separate error interrupt enables such as WRPERRIE, PGSERRIE, STRBERRIE, INCERRIE, and OPERRIE. A failed operation can therefore leave bankStatus stuck at operationOngoing; could the error interrupts be enabled and handled independently of EOP?

Good catch, should be done in the new revision.

[jgmess-dlr]

There is an off-by-one write here: bank is 1 or 2, while bankStatus has indices 0 and 1. Bank 1 updates the wrong slot and bank 2 writes past the array. This should use bankStatus[bank - 1].

Done.

[jgmess-dlr]

There is an off-by-one write here: bank is 1 or 2, while bankStatus has indices 0 and 1. Bank 1 updates the wrong slot and bank 2 writes past the array. This should use bankStatus[bank - 1].

Fixed.

[sylvesterkaczmarek]

Rechecked the current head. Two compile blockers remain in the new helpers: initErase(uint32_t volatile& CR) references index although it has no index parameter, and initiateProgram() uses sizeof(uin32_t). Please fix both before merge.

[sylvesterkaczmarek]

The two compile blockers from my previous review are fixed. One correctness issue remains in the new program bounds check: using addr + FlashWord * sizeof(uint32_t) >= OriginAddr + Size rejects the final valid flash word, whose end is exactly OriginAddr + Size. Please use > (or an equivalent subtraction-based check) so the last flash word remains writable.

[jgmess-dlr]

The two compile blockers from my previous review are fixed. One correctness issue remains in the new program bounds check: using addr + FlashWord * sizeof(uint32_t) >= OriginAddr + Size rejects the final valid flash word, whose end is exactly OriginAddr + Size. Please use > (or an equivalent subtraction-based check) so the last flash word remains writable.

Fixed!

[sylvesterkaczmarek]

Rechecked current f66af534. The final flash word is now accepted by the bounds check, and the earlier compile, bank-index and error-interrupt issues from my reviews are addressed. No remaining blocker from me.