Alarm: Simplify alarm alerting screen (#2211) #26
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Simplify alarm alerting screen and fix bug with alerting on time value change SetAlerting creates an lv_task to automatically call StopAlerting after one minute. This task will call an invalid function reference and lead to a crash under the following condition: All exit paths but the time value change (so not considering this fix) call StopAlerting themselves, which also terminates the lv_task. However, the value change callback only calls DisableAlarm, because its normal use case is for setting up an alarm, where you have to re-confirm enabling the alarm after every change you make. DisableAlarm still sets isAlerting in the alarmController to false, probably because someone thought a currently alerting but also disabled alarm makes no sense, this was introduced in a0cd439. That causes the destructor of Alarm to think there is nothing to do regarding the alerting when the alarm screen is dismissed. Therefore it does not call StopAlerting and the lv_task is left with an invalid function pointer, because Alarm does not exist anymore once the lv_task finally goes to call the callback function
|
Build size and comparison to main:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Simplify alarm alerting screen and fix bug with
alerting on time value change
SetAlerting creates an lv_task to automatically call StopAlerting after one minute. This task will call an invalid function reference and lead to a crash under the following condition:
All exit paths but the time value change (so not considering this fix) call StopAlerting themselves, which also terminates the lv_task. However, the value change callback only calls DisableAlarm, because its normal use case is for setting up an alarm, where you have to re-confirm enabling the alarm after every change you make. DisableAlarm still sets isAlerting in the alarmController to false, probably because someone thought a currently alerting but also disabled alarm makes no sense, this was introduced in a0cd439. That causes the destructor of Alarm to think there is nothing to do regarding the alerting when the alarm screen is dismissed. Therefore it does not call StopAlerting and the lv_task is left with an invalid function pointer, because Alarm does not exist anymore once the lv_task finally goes to call the callback function