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.

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

Persistent memory check value verification, defaulting when fails. #662

Merged
merged 9 commits into from
May 28, 2022

Conversation

jboone
Copy link
Contributor

@jboone jboone commented May 28, 2022

Long-overdue enhancement: compute a check value for the contents of the 256-byte persistent RAM (RTC "REGFILE"). Check at power-on, and only use the persisted data if the check value is correct. If it is not, replace with default values and use those. Instead of directly modifying persistent memory, push changes from a local (non-persistent) RAM cache every second, along with an updated check value.

This addresses random issues users have had with brand new PortaPacks that seem dead. In my experience, the hardware is never at fault, but instead the (random) contents of persistent RAM on their HackRFs are causing the firmware to fail to boot.

A side benefit of using a cached copy of persistent settings is that the data structure members in data_t no longer need to be uint32_t aligned and sized. Apparently that's a requirement for RTC REGFILE access -- the Cortex-M will fault. This will allow compacting that structure somewhat, in case more information needs to be stored in persistent RAM.

@jLynx
Copy link
Contributor

jLynx commented May 28, 2022

This looks awesome! I really appreciate you taking the time to do this. I'll do some testing shortly with it. Again, thanks very much

@jimilinuxguy
Copy link
Contributor

Pulling this into my local and building it now.

@jLynx jLynx requested review from jLynx and jimilinuxguy May 28, 2022 05:23
@jLynx jLynx added the enhancement New feature or request label May 28, 2022
Copy link
Contributor

@jLynx jLynx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just tested and seems to be working great for me. Tested the boot config options and also tested that audio from FM radio works. (H1 clone) Approved. But lets wait for some more testers before merging

@jimilinuxguy
Copy link
Contributor

I can verify that my testing did not find any issue on both my h2+ units.

@gullradriel
Copy link
Member

Booting correctly here, looks fine.

@jboone
Copy link
Contributor Author

jboone commented May 28, 2022

I suppose I should’ve thought a bit more about whether I consider this “feature” done before submitting the pull request. There’s a couple of nice-to-haves I didn’t put in, but probably should. One is a revision field in the data structure. So if the structure is modified, it can be incremented so that incompatible data already in the persistent RAM can be migrated or reset to default values. I could also do a compaction pass, where I collapse existing fields that are taking up more space than they need to. I also find the API around the backlight timer to be peculiar — the getter returns seconds, while the setter requires an index into an array that maps indices to seconds. There’s probably other things, too… because I can’t stop fiddling with things. :-)

@jLynx
Copy link
Contributor

jLynx commented May 28, 2022

I suppose I should’ve thought a bit more about whether I consider this “feature” done before submitting the pull request. There’s a couple of nice-to-haves I didn’t put in, but probably should. One is a revision field in the data structure. So if the structure is modified, it can be incremented so that incompatible data already in the persistent RAM can be migrated or reset to default values. I could also do a compaction pass, where I collapse existing fields that are taking up more space than they need to. I also find the API around the backlight timer to be peculiar — the getter returns seconds, while the setter requires an index into an array that maps indices to seconds. There’s probably other things, too… because I can’t stop fiddling with things. :-)

Hey if you are willing to add those nice to have features in that would be awesome! If not then there is no problem as I know you are a busy guy and already appreciate what you have done so far

@gullradriel
Copy link
Member

Please apply that PR as it seem to also solve the boot problem with black/white screens in latest next.
@jimilinuxguy are you reviewing ? :-p

@jboone
Copy link
Contributor Author

jboone commented May 28, 2022

OK, I'll stop for now. At this point, I'm making changes just to make changes, pretending C++ is Rust, and then remembering it's not.

@jLynx
Copy link
Contributor

jLynx commented May 28, 2022

Alright, I'll merge it in now. Fell free to open another PR in the future if you want to add any more changes or fixes

@jLynx jLynx merged commit e5a30b4 into portapack-mayhem:next May 28, 2022
@jboone
Copy link
Contributor Author

jboone commented May 28, 2022

Of course, there's still more I'd do, but those changes are more invasive. Instead of having free functions serving as an API to persistent memory, I would hand out a pointer to a data_t, collect all those free functions into the data_t class, and let callers use that.

zigad pushed a commit to zigad/portapack-mayhem that referenced this pull request Aug 2, 2022
…ortapack-mayhem#662)

* Make default constructor for touch calibration

* Add persistent memory check value and access abstraction.

* Add persistent data_t default constructor with reasonable defaults.

* serial_format_t default constructor.

* Tidy up backlight timeout type.

* Add persistent data struct version/checking.

* Make range_t functions constexpr.

* Move ui_config and functions into class.

* Add backlight_config_t struct, separate enable and time settings.
@Brumi-2021 Brumi-2021 mentioned this pull request Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants