search

pez-globo / pufferfish-software

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

alarm limits from the set_alarms screen are set just by changing the values #331

Closed rohanpurohit closed 3 years ago

rohanpurohit commented 3 years ago

Alarm limits are changed on incrementing/decrementing alarm_limits for spo2 and/or HR from the set alarm limits screen, without pressing apply changes

Steps to reproduce the behavior:

  1. Start Ventilation
  2. go to set alarms screen
  3. increment, decrement spo2 or hr alarms and check event log

Expected behavior alarm limits should be changed only on pressing "apply changes"

Additional context backend running ventserver.simulator , maybe its just related to the frontend? revision: f50a5e84db4307225fe73e02748b4f5b07c6fbbe

maybe a regression from 4ee0ee48d50126cbaefa3867f5128e5861664768

ethanjli commented 3 years ago

I'm able to reproduce this problem from v0.7.0 i.e. f50a5e8, but I'm not able to reproduce it from 29d2cb6 (i.e. prior to 4ee0ee4). This confirms that the issue was surfaced specifically by f50a5e8. I think there's a more fundamental mismatch in how AlarmLimitsRequestStandby, AlarmLimitsRequest, AlarmLimits, and/or initParameterUpdate are being used vs. how I think they're meant to be used, resulting in the changes I made in 4ee0ee4 causing this regression.

Specifically: changing the alarm limits from the Alarm Limits Screen is only supposed to change the internal alarmLimitsRequest state on that screen and the AlarmLimitsRequestStandby in the store. The internal state is only supposed to be committed to AlarmLimitsRequest when the Apply Changes button is clicked, and AlarmLimitsRequestStandby is never supposed to be read outside standby mode. However, while I am adjusting the SpO2 limit from the Alarm Limits Screen on f50a5e8, I'm able to activate and deactivate the SpO2 too high alarms (this is the bug reported in this issue). The fact that #332 fixes this by adding a check on the getIsVentilating selector in a useEffect in ToolBar.tsx, which I incorrectly would've expected to be redundant to the isVentilatorOn state check in ToolBar.tsx, tells me there's something deeper going on, possibly related to the fact that I don't know what useEffect is or how it's being used with initParameterUpdate in ToolBar.tsx. My expectation was that initParameterUpdate is only being called when the StartVentilation button is pressed, since its purpose is to commit ParametersRequestStandby and AlarmLimitsRequestStandby to ParametersRequest and AlarmLimitsRequest only when that button is pressed; but apparently initParameterUpdate is also being called throughout ventilation? This would explain why adding an getIsVentilating selector in #332 to conditionally decide when to call initParameterUpdate from the useEffect works. But then why not just have the Start Ventilation button trigger a direct call to initParameterUpdate, instead of having this whole thing with useEffect? And what's the relationship between the isVentilatorOn state and the getIsVentilating selector? Should we just replace isVentilatorOn with getIsVentilating?

@rohanpurohit I'm going to unlink #332 so that we can this issue open, since it points to deeper things which need to be resolved even after we merge in #332

@Sudhir-dev It would be helpful to have a description of the various mechanisms related to these things, and/or a comment on how things are supposed to work (and, if things work differently in practice, how so).