Closed rohanpurohit closed 3 years ago
Observations from testing:
This pull request introduces 1 alert when merging 1ac2f717a60c9217104b55a26ea3f62ad63cb8bb into aff9ab0865d6cf0218e539c8dffc6d8ab0832432 - view on LGTM.com
new alerts:
The battery alarms look good in testing, for both the firmware and the backend. Right now our implementation assumes that activation of a new alarm event will not cause alarms to be automatically unmuted - I believe that's what we decided to do, so this assumption is reasonable. If one day we decide to make generation of a new alarm event automatically unmute alarms, then when we're charging the battery from wall power and it goes from 5% to 6%, alarms will be unmuted - which is undesirable. I don't think we need to worry about this.
For how the frontend displays the sensor disconnection events, I'd suggest that the event text should be "Internal sensor failed", and then the Details text should say which sensor was lost, e.g. "O2 sensor connection lost". Otherwise, everything looks good to merge. For records-keeping:
Added change for label and detail
This PR adds: