Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Flashfs Loop recording and initial erase #209

Merged
merged 2 commits into from
Jan 18, 2025
Merged

Flashfs Loop recording and initial erase #209

merged 2 commits into from
Jan 18, 2025

Conversation

gongtao0607
Copy link
Contributor

@gongtao0607 gongtao0607 commented Dec 22, 2024 •
edited
Loading

This PR implements: #126 bullet 1 and 2:

  • Flashfs is now a circular volume with a head pointer and tail pointer.
  • Initial erase setting allows flashfs to automatically erase a few sectors upon logging start.

set blackbox_initial_erase = 8388608 (size in bytes) to turn it on, default is 0 (turned off).

Both are tested on Flydragon and Matek G474Heli as my daily firmware

@gongtao0607 gongtao0607 marked this pull request as ready for review December 24, 2024 08:36
@gongtao0607 gongtao0607 mentioned this pull request Dec 26, 2024
Closed
@rotorflight
Copy link
Owner

This really needs to get tested with other FCs that have a NOR flash.

@gongtao0607
Copy link
Contributor Author

I do not have any other FCs. Could you help test?

@robthomson
Copy link
Contributor

Happy to test.

@gongtao0607
Copy link
Contributor Author

Happy to test.

You can test this PR build on master, or the following build which is essentially RF-4.4.x + #209 + #211 + #212 all together:
https://github.com/gongtao0607/rotorflight-firmware/actions/runs/12520725507

@rotorflight
Copy link
Owner

rotorflight commented Dec 28, 2024 •
edited
Loading

This is an absolutely fantastic feature. Let's see if it works with BF boards. Most of them have a NOR flash.
Lots of people are using BF boards, so we can't really kill them off.

Also... the NOR chips are usually much smaller, so another question is if they should be fully erased at arming.
Typical NOR chips are 64Mbit and 128Mbit, so only 8Mb and 16Mb. Just about enough for one flight.

@gongtao0607
Copy link
Contributor Author

Also... the NOR chips are usually much smaller, so another question is if they should be fully erased at arming.
Typical NOR chips are 64Mbit and 128Mbit, so only 8Mb and 16Mb. Just about enough for one flight.

If NOR flashes erase considerably faster in multi-block erase (or full chip erase) than serialized block erase -- I'm not sure if it's true --, it worth improve the erasing logic. Otherwise, the current logic provides enough flexibility for either full chip, 1/2 chip or whatever threshold erase.

@robthomson
Copy link
Contributor

I have one BF board I can test on.

https://www.jhemcu.com/e_productshow/?73-GF30F722-ICM-73.html

How do I know if it has a NOR chip?

@gongtao0607
Copy link
Contributor Author

gongtao0607 commented Dec 28, 2024 •
edited
Loading

I have one BF board I can test on.

https://www.jhemcu.com/e_productshow/?73-GF30F722-ICM-73.html

How do I know if it has a NOR chip?

https://github.com/rotorflight/rotorflight-firmware/blob/master/src/main/target/JHEF7DUAL/target.h#L125
Looks like a 16M M25P16

This will be the most interesting one to test.

@gongtao0607
Copy link
Contributor Author

I'm getting very confused:

  • The image of your board clearly shows a Winbond flash chip. In the spec it says 16 MiB flash.
  • The provided config for this board is M25P16
    • M25P16 is a 16Mib (=2MiB) flash chip. Unless it's referring to a general type??
  • In the M25P16 driver it has a list of chips that it can detect. Filter with (Winbond, 16MiB chip), I get W25Q128 code
  • In the same folder, a separate w25q128 driver detects the same chip code

It seems very unorganized.

@rotorflight
Copy link
Owner

rotorflight commented Dec 28, 2024 •
edited
Loading

I doubt any BF board is using 16 Mbit chips. That's too small even for BF.
Usually they are 64Mbit or 128Mbit. Occasionally larger.
There are hardly any BF boards using NAND flash. It wasn't working in BF until just recently.
I fixed the driver in RF when we needed more space.

@rotorflight
Copy link
Owner

It seems very unorganized.

Welcome to the world of BF

@gongtao0607
Copy link
Contributor Author

gongtao0607 commented Dec 30, 2024 •
edited
Loading

Worst case NOR users get these advanced features disabled by default but they have nothing to lose.

They have to figure out an acceptable arm delay v.s. log file size. Or the amount of sensor data to record (data rate) v.s. sector erase speed (background erase).

@rotorflight
Copy link
Owner

I don't think we can leave this to end users to figure out.
It either works with NOR flashes, or the feature should be disabled with them.

@gongtao0607 gongtao0607 force-pushed the flashfs_arming_erase branch 2 times, most recently from e65ca9c to 4cfb987 Compare January 3, 2025 00:23
@gongtao0607
flashfs: add flashfs loop support and initial erase
080dec0 
When using loop mode, we will identify the "start of the data stream"
(by empty region followed by filled region). Also the usable size is
reduced to `full size - page size - buffer size`. (The - buffer size is
for code simplicity).
The original FLASHFS_FREE_BLOCK_SIZE is changed to page size to avoid
misalignment.

A new option blackbox_initial_erase is added to maintain a certain amount
of free space upon record start.

This also add (change) some cli cmds for diagnoses:
flash_fill
flash_verify
flash_erase_sector
flashfs_initial_erase
@gongtao0607 gongtao0607 force-pushed the flashfs_arming_erase branch from 4cfb987 to 080dec0 Compare January 6, 2025 04:26
@gongtao0607 gongtao0607 changed the title Flashfs Loop recording and arming erase Flashfs Loop recording and initial erase Jan 6, 2025
@gongtao0607
Copy link
Contributor Author

Tested on flywing f405: it will take 15s to complete a 4MiB initial erase. All other functionalities are normal and uninterrupted. The recording won't start until the erase is complete.

rotorflight
rotorflight previously approved these changes Jan 15, 2025
View reviewed changes
@rotorflight
Copy link
Owner

Well, that is a rather long time, but the user can choose not to do that.

@rotorflight
Copy link
Owner

rotorflight commented Jan 16, 2025 •
edited
Loading

I am considering if the blackbox_initial_erase = 8388608 should be in kilobytes instead of bytes.
People don't like scary large numbers. Especially if they dont understand why it is 8388608 and not 8388609 etc...
And if it is in kilobytes, maybe the data type can be U16 instead of U32.

@gongtao0607
Initial erase: change param unit to KiB
28e92d0 
Change `initialEraseFreeSpace` to `....KiB` and update unit tests.
@gongtao0607
Copy link
Contributor Author

I am considering if the blackbox_initial_erase = 8388608 should be in kilobytes instead of bytes. People don't like scary large numbers. Especially if they dont understand why it is 8388608 and not 8388609 etc... And if it is in kilobytes, maybe the data type can be U16 instead of U32.

28e92d0 (Don't think it saves any byte though.)

@rotorflight rotorflight merged commit 5fc85a8 into rotorflight:master Jan 18, 2025
1 check passed
gongtao0607 added a commit to gongtao0607/rotorflight-firmware that referenced this pull request Jan 18, 2025
@gongtao0607
Flashfs Loop recording and initial erase (rotorflight#209)
9ba29c1 
* flashfs: add flashfs loop support and initial erase

When using loop mode, we will identify the "start of the data stream"
(by empty region followed by filled region). Also the usable size is
reduced to `full size - page size - buffer size`. (The - buffer size is
for code simplicity).
The original FLASHFS_FREE_BLOCK_SIZE is changed to page size to avoid
misalignment.

A new option blackbox_initial_erase is added to maintain a certain amount
of free space upon record start.

This also add (change) some cli cmds for diagnoses:
flash_fill
flash_verify
flash_erase_sector
flashfs_initial_erase

* Initial erase: change param unit to KiB

Change `initialEraseFreeSpace` to `....KiB` and update unit tests.
@gongtao0607 gongtao0607 deleted the flashfs_arming_erase branch January 18, 2025 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rotorflight rotorflight rotorflight approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gongtao0607 @rotorflight @robthomson