Skip to content

This gets called from ISR currently #81

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

antisvin
Copy link
Collaborator

We store settings from the USB interrupt and current code hangs when disabling interrupts in the old function. AFAICT calling it not from an ISR would be safe too.

@pingdynasty
Copy link
Collaborator

Good find. What do you think about moving the critical lock out of the function instead, like this:
44ea65c

@antisvin
Copy link
Collaborator Author

Well considering that this left saving settings broken for a year or so, I had no choice but debug this.

Regarding linked branch, there are some minor concerns:

  1. Critical section disables interrupts and we want to make it as short as possible. This update makes CS larger, so probably not a good thing.
  2. Moving CS to a higher level means that underlying code (Storage::writeResource()) becomes not interrupt-safe, so it should be documented. It becomes caller's responsibility to add a CS around it and it's easy to forget about this
  3. Is it necessary to place registry.init(); in that CS too? I would guess it doesn't need it as it only reads from flash.

@pingdynasty
Copy link
Collaborator

Concerns noted.
The code is called from two places:
When saving a patch, no patch should be running and a longer CS is not a problem. Including registry.init() in the CS means that any attempt to change the patch while storing a new one should not be a problem.
When saving settings, worst case I figure there may be an audio drop, though the saved data is very small so this is probably unlikely. Regardless I don't think saving system settings is something a user needs to do mid set.

@pingdynasty
Copy link
Collaborator

I cut a release candidate, please help test: https://github.com/RebelTechnology/OpenWare/releases/tag/v22.5.rc4

@antisvin
Copy link
Collaborator Author

antisvin commented Jan 4, 2023

When saving settings, worst case I figure there may be an audio drop, though the saved data is very small so this is probably unlikely.

This used to be true with the previous code version, but in that branch the critical section would include the call to Storage::defrag and it's not very fast with external flash.

The PR ended up with a conflict apparent apparently - https://github.com/RebelTechnology/OpenWare/pull/81/conflicts . I'd say it's worth to try making a call for testing on forum as it resolves issues quite a few people have ran into.

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.

2 participants