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

[FL-3765][FL-3737] Desktop: ensure that animation is unloaded before app start #3569

Merged
merged 2 commits into from
Apr 4, 2024

Conversation

DrZlo13
Copy link
Member

@DrZlo13 DrZlo13 commented Apr 4, 2024

What's new

  • Animation fully unloaded before app starts.
  • Animation is unloaded only if it was loaded before.

Verification

  • Check that the log line [I][AnimationManager] Unloading animation 'xxx' occurs before any application log lines.
  • Build fw with LOADER_AUTOSTART flag that pointed to some app, check that the firmware does not crash.

Checklist (For Reviewer)

  • PR has description of feature/bug or link to Confluence/Jira task
  • Description contains actions to verify feature/bugfix
  • I've built this code, uploaded it to the device and verified feature/bugfix

Copy link

github-actions bot commented Apr 4, 2024

Compiled f7 firmware for commit a5a44287:

@DrZlo13 DrZlo13 changed the title [FL-3765] Desktop: ensure that animation is unloaded before app start [FL-3765][FL-3737] Desktop: ensure that animation is unloaded before app start Apr 4, 2024
@skotopes skotopes merged commit 5431257 into dev Apr 4, 2024
11 checks passed
@skotopes skotopes deleted the zlo/3765-fix-animation-unload branch April 4, 2024 12:42
@Willy-JL
Copy link
Contributor

Willy-JL commented Apr 4, 2024

I understand the logic is, loader callback sends the event and acquires the semaphore, then the event handler frees the animation and releases the semaphore... but a semaphore does not wait on acquire... a semaphore will wait on acquire only if it's limit is reached, which it is not... I don't think a semaphore is the correct data structure here, a FuriApiLock would be more suited... and in fact, using the semaphore still causes crashes when opening NFC app with big animations loaded... I am currently verifying if my FuriApiLock approach resolves the problem, I will make a PR if so

@skotopes
Copy link
Member

skotopes commented Apr 5, 2024

@Willy-JL semaphore is kinda essential there. Maybe crash on timeout is not necessary, wait forever will be ok. Also semaphore is allocated empty, so whole thing is working as intended: signal event, wait for event be processed, release semaphore, then return.

We've been testing whole thing with various animations and there were no problem. Also check if you merged latest version of this PR and not intermediate one.

@Willy-JL
Copy link
Contributor

Willy-JL commented Apr 5, 2024

@skotopes My point is that a semaphore does not wait for release here... semaphore does acquire, then loader continues... because the semaphore is not at max value... so it acquires the semaphore, and continues with absolutely no wait. There would be wait if this function was called twice without releasing the semaphore, then the first acquire would pass, the second acquire would wait. But here every single acquire is matched by a release, so all of the acquires will pass instantly without waiting for release.

@skotopes
Copy link
Member

skotopes commented Apr 5, 2024

desktop->animation_semaphore = furi_semaphore_alloc(1, 0); semaphore is allocated in empty state, acquire will wait for someone to release it

@Willy-JL
Copy link
Contributor

Willy-JL commented Apr 5, 2024

Apologies, I had the wrong understanding of how the semaphore counter works. You're totally right, sorry!

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.

3 participants