qmk_firmware icon indicating copy to clipboard operation
qmk_firmware copied to clipboard

Fix flash wear leveling sector calculation

Open Diff-fusion opened this issue 11 months ago • 3 comments

Description

Currently the calculation of the last usable flash sector doesn't work properly if last_sector starts beyond the end of the actual flash size: https://github.com/qmk/qmk_firmware/blob/172c3496756fb5468c778c2536b4334f25eb883b/platforms/chibios/drivers/wear_leveling/wear_leveling_efl.c#L87-L92 Here the last_sector is calculated from the first_sector but also i is included in the calculation. But i is incremented in each loop iteration and thus an increasing offset is added. The solution here is to first calculate the number of the last usable sector and then use another loop to calculate the first sector.

Types of Changes

  • [ ] Core
  • [x] Bugfix
  • [ ] New feature
  • [ ] Enhancement/optimization
  • [ ] Keyboard (addition or update)
  • [ ] Keymap/layout (addition or update)
  • [ ] Documentation

Issues Fixed or Closed by This PR

Checklist

  • [x] My code follows the code style of this project: C, Python
  • [x] I have read the PR Checklist document and have made the appropriate changes.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [x] I have read the CONTRIBUTING document.
  • [ ] I have added tests to cover my changes.
  • [x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Diff-fusion avatar Jan 02 '25 12:01 Diff-fusion

Thanks for the review. I fixed the issues you mentioned and added some additional checks. One remaining issue is that flash sectors that are used for the firmware code could be used by the wear_leveling driver. I don't see any checks to prevent that. But I'm not that experienced with QMK and don't know what would be the best way to do this. Error handling is a bit problematic. Exiting with chSysHalt is better than just crashing. But maybe compile time checks could be used to prevent some of the error cases.

Diff-fusion avatar Jan 04 '25 11:01 Diff-fusion

Hey, is there anything missing to get this merged?

Diff-fusion avatar Feb 03 '25 22:02 Diff-fusion

Hey, is there anything missing to get this merged?

From you, nothing at this stage. Needs two approving reviews for merge; we'll get to it when we can.

tzarc avatar Feb 05 '25 09:02 tzarc

Thanks for finally merging this :+1:

Diff-fusion avatar Dec 11 '25 20:12 Diff-fusion