pez-globo / pufferfish-software

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

Update the active simulator before updating the clock #272

Closed ethanjli closed 3 years ago

ethanjli commented 3 years ago

This PR fixes incorrect behavior in clock updates for the breathing circuit simulator of the frontend. Previously, the Simulators::transform method was updating the clock before updating the pointer to the active simulator. However, the clock update method was returning early if there was no active simulator when it was called, which meant that a newly-activated simulator's transform method would be called with a stale clock in the first call. Calling the clock update method after updating the pointer to the active simulator should fix this problem.

ethanjli commented 3 years ago

@rohanpurohit The Github Actions checks have passed on this PR. I suspect the problems you're getting in #269 are related to some CMakeLists changes - I'll follow up on them there.

Once you confirm that this change is what is needed for firmware tests to run correctly, please do a approval and then I'll squash-and-merge into develop.

rohanpurohit commented 3 years ago

Test case currently failing: https://github.com/pez-globo/pufferfish-software/blob/feature/firmware-test-driver/firmware/ventilator-controller-stm32/Core/Test/Pufferfish/Driver/BreathingCircuit/Simulator.cpp#L314

https://github.com/pez-globo/pufferfish-software/runs/1624837950#step:6:20

so here i'm testing transform_spo2 method in HFNCSimulator::transform

so in each test case to update the current time we need to call transform method in simulator https://github.com/pez-globo/pufferfish-software/blob/develop/firmware/ventilator-controller-stm32/Core/Src/Pufferfish/Driver/BreathingCircuit/Simulator.cpp#L168 twice as initial time is 0 ,

example: https://github.com/pez-globo/pufferfish-software/blob/feature/firmware-test-driver/firmware/ventilator-controller-stm32/Core/Test/Pufferfish/Driver/BreathingCircuit/Simulator.cpp#L42

before the suggested fix c33d11a3bb9a1af312f859a2ca62df732758c532, we were calling transform 3 times for the current time to update

which i think maybe causing the cases to fail randomly , maybe i should change the "then" statement please review and suggest any change if needed

ethanjli commented 3 years ago

I'm not sure I understand the intention of the first test you linked to at https://github.com/pez-globo/pufferfish-software/blob/feature/firmware-test-driver/firmware/ventilator-controller-stm32/Core/Test/Pufferfish/Driver/BreathingCircuit/Simulator.cpp#L314 (Given a hfnc simulator object, when SPO2 is calculated, then spo2 is less than spo2_min) or how it's related to the clock update.

As for https://github.com/pez-globo/pufferfish-software/blob/feature/firmware-test-driver/firmware/ventilator-controller-stm32/Core/Test/Pufferfish/Driver/BreathingCircuit/Simulator.cpp#L42 , it makes sense to me that we should just call simulators.transform twice; the first time to initialize the clock from 0 to a valid value (in the test it initializes to 12), and the second time to perform another update. If you check sensor_measurements.time between the two calls, do you get 0? Also, where are you seeing test cases fail randomly? What kind of randomness is this (e.g. is failure/success different each time Github Actions runs the tests?)?

rohanpurohit commented 3 years ago

in that particular test case i'm testing https://github.com/pez-globo/pufferfish-software/blob/develop/firmware/ventilator-controller-stm32/Core/Src/Pufferfish/Driver/BreathingCircuit/Simulator.cpp#L136 this method. the test case in question fails/passes randomly, so if i run coverage multiple times it'll pass , therefore was wondering if that maybe linked to us calling transform multiple times , but yes the hotfix definetly fixes the behaviour where we had to call it thrice

i'll go through again why it fails and report back. meanwhile i think we can go ahead with the hotfix.