pez-globo / pufferfish-software

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

Remove clock from Redux store #417

Closed rohanpurohit closed 2 years ago

rohanpurohit commented 2 years ago

This PR:

add temporary saga code to detect backend connection loss.

TODO (ethan/rohan):

rohanpurohit commented 2 years ago

I did not really give much thought to my code for saga! it does work but I'm sure there is a better way to do things! @ethanjli maybe you can co-author that part??

Testing:

rohanpurohit commented 2 years ago

We'll also need to add the heartbeat-related functionality back as a saga

I don't think I completely understood this, just add back what I deleted, or add a generator function that dispatches the action in saga.ts? btw I deleted that thinking even that spams a lot and doesn't look like we are using it anywhere, but I'm guessing we might need it just in case or for future features?

rohanpurohit commented 2 years ago

saw this TODO and moved backend stuff from app to connection

rohanpurohit commented 2 years ago

@ethanjli can you take a look at the latest changes for saga? so basically I'm checking if we have a STATE_UPDATED in a time period of 3 seconds, if we don't have that then we dispatch BACKEND_CONNECTION_DOWN, since we have this I'm assuming we can now remove the BACKEND_HEARTBEAT actions?

ethanjli commented 2 years ago

CPU usage measurements after running the frontend dashboard with the simulator backend on the RPi for at least 10 minutes:

If end-user latency looks good to you, I think this concludes the performance optimization on the frontend for running in the dashboard; in future commits we just need to check against performance regressions. I think it's still worth fixing the issues of massive redraws for the alarm border, modals, etc., in order to improve responsiveness to user actions. Regardless, for HFNC mode we need to fix the issue of the red border flashing on the quickstart screen (and being slow in doing that) when we stop ventilation.

CPU usage in the Python backend server is still very high, and based on what we've seen in this PR I suspect the culprit is that the backend is polling on clock updates (like what we removed with redux + sagas in this PR) every millisecond (!!!). The simple solution would be to accept higher clock jitter, e.g. by polling only every 2 ms or every 5 ms; the more efficient, more accurate, but potentially much more complicated solution, would be to have all timing-related filters (state synchronizers, backend protocol, server protocol) report the next clock updates they need, and then ventserver.integration._trio.process_clock would sleep for variable durations and wake for the deadline of each requested clock update.

rohanpurohit commented 2 years ago

Looks good to me in terms of latency!

  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.
ethanjli commented 2 years ago

Moving the connection-specific code from store/controller into store/connection turned into a giant renaming operation because then it also made sense to move the protobuf files (and some associated type definitions) from store/connection/proto to store/proto, so a lot of frontend files have been touched. I've tried to restrain myself by not doing more moving of code or renaming of things beyond that, since those things should really be saved for a future PR. @rohanpurohit Can you try to go to all of the various frontend screens and press all of the various buttons to ensure that the frontend doesn't break (e.g. with console.log errors or with typescript/eslint/etc. complaints in yarn start) on these latest changes? Once it looks good to you, you can go ahead and merge this PR.

rohanpurohit commented 2 years ago

Moving the connection-specific code from store/controller into store/connection turned into a giant renaming operation because then it also made sense to move the protobuf files (and some associated type definitions) from store/connection/proto to store/proto, so a lot of frontend files have been touched. I've tried to restrain myself by not doing more moving of code or renaming of things beyond that, since those things should really be saved for a future PR. @rohanpurohit Can you try to go to all of the various frontend screens and press all of the various buttons to ensure that the frontend doesn't break (e.g. with console.log errors or with typescript/eslint/etc. complaints in yarn start) on these latest changes? Once it looks good to you, you can go ahead and merge this PR.

mostly tried everything! looks good to me. merging