pez-globo / pufferfish-software

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

Make backend activate alarm when MCU or frontend is lost #344

Closed ethanjli closed 3 years ago

ethanjli commented 3 years ago

This PR fixes #320 by:

This PR also fixes #343 by:

This PR also reorganizes the modules inside ventserver.protocols by dividing them into subpackages based on their function; this requires changes to almost every module and unit test in the backend. Refer to the module docstring in ventserver.protocols.__init__ for more information.

ethanjli commented 3 years ago

@rohanpurohit Since this PR touches a bunch of files, it'd be great if you could test whatever you can to help look for any regressions introduced by the refactoring and the new functionality, as well as any gaps in the new functionality. After light to moderate testing, let's merge it in as prototype-quality code, and we'll probably discover more subtle bugs/regressions afterwards which we can fix in future PRs. For records-keeping:

  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
rohanpurohit commented 3 years ago

On light to moderate testing, everything works as expected

observations on a Nucleo board with RPI :

2021-05-14 12:39:47,637 ventserver.io.subprocess.frozen_frontend WARNING No message received from frontend for more than a 1s; killing frontend process: CompletedProcess(args=['killall', '/usr/lib/chromium-browser/chromium-browser-v7'], returncode=0)

the frontend was just started while the backend was running application and connected to MCU, although other factors could have caused this therefore I'd say we can be more clear once it's reproducible (one thing to note: our MCU was not connected to any sensors, and the initializables were disabled in main.cpp) because of factors like this I wouldn't be too concerned about this issue, and if it's reproducible we can check later. I did try resetting the RPI but still faced the same issue, my guess is probably the MCU and the test circumstances. could not test more thoroughly because of this, I will be getting the board from raavi to test/work on the firmware side soon.

lgtm-com[bot] commented 3 years ago

This pull request introduces 3 alerts and fixes 2 when merging dd9b1011acc7faa787caf46b1c9fed8bcbc065ba into cc2d07065ca15aa6bf78bff8b6e7a0458524e1b9 - view on LGTM.com

new alerts:

fixed alerts:

ethanjli commented 3 years ago

I wondered if we should disable anything on the frontend if the connection to the MCU is lost? maybe pause ventilation to loading?

Good point - we'll need to explore the implications of this before going ahead, but it would make the MCU-lost situation behave more consistently with the backend-lost situation.

the frontend was just started while the backend was running application and connected to MCU

This could happen if the frontend establishes its websocket connection and then has a >2 s delay before it starts sending messages to the backend; one possible explanation for this is that PR #340 made the frontend start with null as the value for all of the states it could send to the backend, so the frontend must be initialized by the backend before it starts receiving messages from the backend; and the backend has quite a few messages to send now, so it might take a few seconds in the worst case before the frontend has anything to send to the backend. I haven't seen this behavior on my end, but also I usually start the backend after the frontend is loaded so I haven't really tested this condition. Let's create a Github issue on this if we observe it happening in the future - it might be fixed by sending a message to the backend with some non-null-initialized state in the frontend, e.g. a version identifier in the frontend.

Based on the findings in light-to-moderate testing, I will merge in this PR now.