search

pez-globo / pufferfish-software

All software for the Pufferfish ventilator.
Apache License 2.0
0 stars 1 forks source link

Implement alarm mute timeout countdown #370

Closed rohanpurohit closed 3 years ago

rohanpurohit commented 3 years ago

This PR includes:

rohanpurohit commented 3 years ago

@ethanjli this PR is ready for testing/code review

ethanjli commented 3 years ago

Here are my results from testing the frontend with the firmware:

rohanpurohit commented 3 years ago

Here are my results from testing the frontend with the firmware:

  • When I press the "alarm mute" button, the countdown time decrements by 1 s, at an interval of 1 s. When I modify the backend to print all AlarmMute messages it sends to the frontend, I see that the backend always sends at least 3 messages (usually 4 messages) for each second of the remaining countdown; and currently the backend's schedule of messages to send to the frontend has AlarmMute being sent once every 230 ms. So actually the frequent & regular updates of the countdown time with our current state synchronization schedule is correct behavior, and whatever bug you had reported previously about seeing delays and large jumps in countdown time must have been from some other bug. We should figure out what was the root cause of that behavior and how it got fixed between your first commit on this PR and your most recent commit on this PR. This also means that #372 may be less urgent.

Note: this was due to frontend lagging most probably because of chromium developer tools open with redux tab, closing the same resulted in expected normal countdown.

  • If I unplug the MCU and plug it back in while the alarm mute is running, the alarms stay muted and the countdown time resets to 2 min (I believe we want the alarm muting to be cancelled in the frontend, backend, and firmware when a device is disconnected, it looks like that's part of #372 so it's for a future PR to address) and the alarm unmute button is disabled (this is a bug; I don't know if it's caused by this PR or if it existed previously)

@ethanjli I cross-checked eventsLog > nextLogEvents > elements array in redux dev tools and found that mcu_connection_down which is 130, persists even after the connection to the firmware is restored, which explains why the mute button is always disabled after starting with application -> unplugging -> and plugging back in as the selector, I added checks for the mcu_connection_down code in nextlogevents, if this is intended or unavoidable then makes sense to use protobuf messages later, if it's an easy fix then maybe we can implement the disable mute button on connection lost in this PR itself, and later just update the selector implementation for protobuf message for connections. thoughts?

ethanjli commented 3 years ago

Regarding the mcu_connection_down issue: I think it's reasonable to leave your getFirmwareDisconnected selector in place with a TODO comment about when it returns the wrong values (i.e. after firmware has been reconnected) and what the fix is (i.e. having a separate protobuf message), and then we can merge in current (incorrect) implementation as a placeholder in this PR.

Here are my findings from more thorough testing:

rohanpurohit commented 3 years ago

Regarding the mcu_connection_down issue: I think it's reasonable to leave your getFirmwareDisconnected selector in place with a TODO comment about when it returns the wrong values (i.e. after firmware has been reconnected) and what the fix is (i.e. having a separate protobuf message), and then we can merge in current (incorrect) implementation as a placeholder in this PR.

Here are my findings from more thorough testing:

  • When I try to run the frontend and application backend with the firmware, the frontend encounters some sort of fatal error when mcu_pb.ts attempts to serialize message.remaining as a uint64. I suspect this is because the frontend is leaving message.remaining == undefined upon startup (which is what I see in the Redux console). I think we'll need to make the application backend track AlarmMuteRequest as a message to save and load from the filesystem, by adding it to protocols.backend.states.FILE_INPUT_TYPES. We'll also need a way to initialize AlarmMuteRequest if the file isn't already on the filesystem; this is a bit complicated, so I've created PR #377 to fix this for you - I tried merging that branch into my local copy of this PR and found it solved the problem, so see if it also works for you.

Thanks for making that change! it works fine for me too

  • After fixing the above issue on my local copy of the repo, I tested the frontend and backend with the firmware, and I found that when I press the mute button the countdown always stays stuck at 02:00.

Yes! seems like after I changed remaining from float to uint64_t, it has stopped working. I'll fix the service and the rest of the issues. thanks

rohanpurohit commented 3 years ago

I forget if we discussed what behavior we want to have when an alarm mute is active and then all alarms become inactive. Maybe we decided that the alarm mute should remain active even when all alarms are deactivated?

I don't recall discussing this particular scenario, mainly we had discussed if alarm mute is active, any new alarm that is triggered should also be muted until the countdown is over, which wasn't the case earlier and was fixed in #348

If so, then I think the countdown and "stop muting alarms" button should always be visible and active whenever an alarm mute is active. This may need to be handled in a separate PR.

I agree, otherwise, the countdown just runs down silently, maybe we could display something like "No Alarms Active" along with the countdown

ethanjli commented 3 years ago

What I see in testing is that, even after debouncing, a value might temporarily return to within the alarm limits range for several seconds (which is enough to make it through the debouncing filter) and then go back out of range. I don't think we would want this to cancel the alarm mute early, it could be annoying. So let's keep the mute going even after all alarms are inactivated. This way we'll have a very simple description for the desired software behavior for alarm mute initiation and cancellation.

A temporary alarm mute period is cancelled and reset exactly when:

  1. The user presses the unmute button from the touchscreen display or the membrane button (manual cancellation),
  2. The 2-minute deadline on the temporary mute period is reached (automatic cancellation from timeout), or
  3. Something has gone very wrong with the system and needs immediate attention: a piece of software has crashed or frozen, or a connection has been lost (automatic cancellation from severe system fault)

The user can press the mute button whenever both of the following requirements are met:

  1. Alarms are not muted (even if there are not currently any active alarms)
  2. All software and connections are running normally

Whenever alarms are muted, the user can always see the countdown to the deadline for automatic cancellation from timeout, and the user can always press the unmute button.

rohanpurohit commented 3 years ago

@ethanjli I have resolved the issues you faced in the last test! along with that, I have added changes for the alert to stay active without any alarms, the part about the mute button being accessible can be done in a future PR, where we have better selectors for all connections.

rohanpurohit commented 3 years ago
  • When the simulator backend is disconnected while alarms are muted, the countdown timer resets to 02:00 and starts counting down from there, instead of continuing the countdown. The alarm mute cancels once it reaches 00:00; however, when I press the mute button again afterwards, the countdown timer still alternates between counting down from 02:00 and counting down from 59:59; and after that countdown finishes, pressing the mute button again causes the countdown timer to alternate between three countdown times. Maybe you're missing a clearTimeout(timer) call somewhere else? Also, if alarms are muted and the backend is disconnected and reconnected, the countdown timer resets to counting down from 02:00 after the backend is reconnected. We probably don't need to fix these buggy behaviors in this PR, since #372 will cancel the alarm mute when the backend is disconnected. However, in the PR which addresses #372, we need to test whether these behaviors still occur.

I just pushed a change to continue countdown, and clearTimeouts where needed, although I'm not entirely sure if it has fixed all the issues stated here.

  • Everything seems to work fine with the firmware. I haven't tested any behaviors when the firmware is disconnected - I don't think this PR has code related to that condition, so it will be tested in the PR for #372.

Yes we'll just have to run the same countdown for firmware disconnection once we have a reliable selector for it, that's all. so yes it doesn't work currently and only runs when backend is disconnected.

  1. This project is licensed under Apache License v2.0 for any software, and Solderpad Hardware License v2.1 for any hardware - do you agree that your contributions to this project will be under these licenses, too? Yes.
  2. Were any of these contributions also part of work you did for an employer or a client? No.
  3. Does this work include, or is it based on, any third-party work which you did not create? No.