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

Save settings on change only / All builds OK (#1699) #1704

Merged
merged 4 commits into from Jun 18, 2023
Merged

Conversation

ia
Copy link
Collaborator

@ia ia commented Jun 14, 2023

Full description is provided here. With big help from Ralim, use of extra variable currentSettings has been removed. But I had to rearrange headers' content a bit.

As I mentioned before source tree of v2.21 release branch has been fully stuffed for TS80P with features. However when I applied changes to dev, I discovered some room for modifications - I did manage to get all builds successfully (S60 included). Full log here.

Is it good enough to merge? Do I have change something? Can't wait any feedback. Thanks.

@Ralim
Copy link
Owner

Ralim commented Jun 14, 2023

Heya,

Super glad the memcmp call worked for this.
Just wanted to confirm what devices you have tested this on (if any).
Mostly want to triple check PinecilV2, fairly sure the flash is remapped to 0x0 as per normal but will test if you haven't :).

But yep looks great to me.

Copy link
Owner

@Ralim Ralim left a comment

Choose a reason for hiding this comment

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

Looks mint to me, just want to check testing.

@ia
Copy link
Collaborator Author

ia commented Jun 14, 2023

Sorry for a delayed response, just wanted to check & double-check some stuff. Here is the thing. I have only one unit of supported hardware which is TS80P. And I'm not sure of how stable dev branch is (is it?). So what I could and did test on my side:

  • all builds with this patch on dev branch for every supported unit per every language: successfully
  • model=TS80P firmware-EN build (with this patch on v2.21 tagged source tree) has been tested physically on my TS80P unit: successfully

I may be not aware of some very tricky low level memory management nuances on MCUs. But as far as I understand since flash_read_buffer implementations read settings page through memcpy from memory address directly, and it works, then using the same approach for memcmp should work as well... right? 😅

@Ralim
Copy link
Owner

Ralim commented Jun 18, 2023

dev branch should be stable. Generally only bugs expected there are non-critical things like an odd glitch here or there.

Thanks for the info, tested this locally and all seems good so going to merge it in.
There shouldn't be any magic low level magic for memory maps on these devices. But I prefer to be cautious till I check it out, but your call on the reading proves how much I've forgotten and yep that does indeed confirm we can do this.

@Ralim Ralim enabled auto-merge (squash) June 18, 2023 04:30
@Ralim Ralim merged commit 4d7e4f4 into Ralim:dev Jun 18, 2023
11 checks passed
@ia ia deleted the dev-settings branch June 21, 2023 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants