Improve some of the driver interfaces for the Nonin sensor

[rohanpurohit]

This PR includes:

• Device.h

  • split invalid_checksum status to invalid_header and invalid_checksum

• FrameBuffer.h

  • Add two different enums BufferInputStatus and BufferOutputStatus
  • add output_ready status for input

• FrameReceiver.h

  • change variable start_of_frame_status_ to at_start_of_frame
  • split validate_start_of_frame into validate_frame_header and validate_frame_checksum

• Types.h

  • Add inner namespace FrameBytes for the 5 bytes in a frame: header,status,pleth,data,checksum
  • Add BIT0 of status byte mentioned as SYNC in datasheet in StatusMasks

In Cpp files resolved most of the unresolved comments from #421 except the refactor for Sensor.cpp which i will also do in this PR itself.

[rohanpurohit]

@ethanjli I was working on the high-level driver and I'm not too sure when we should say the sensor is connected, and unfortunately I broke the connector that connects the nonin module to the MCU breadboard, so I won't be able to test the latest changes until I'm able to get the replacement for the connector wire. sorry about that!

[ethanjli]

Ok, no worries! As for connectivity, I think there are a few levels and we'll need to report the different levels differently. We should consider the module connected to the MCU when we're receiving new packets/samples at the expected interval (I believe it's a few times per second). We should consider the finger sensor connected to the module when the module packets are reporting that the SNSD status flag is false. We should consider the patient's finger connected to the sensor when the module packets are reporting that SpO2 and HR are not at their "missing data" indicator valued (127 and 511, respectively) and simultaneously reporting that the OOT status flag is false.

[rohanpurohit]

Ok, no worries! As for connectivity, I think there are a few levels and we'll need to report the different levels differently. We should consider the module connected to the MCU when we're receiving new packets/samples at the expected interval (I believe it's a few times per second). We should consider the finger sensor connected to the module when the module packets are reporting that the SNSD status flag is false. We should consider the patient's finger connected to the sensor when the module packets are reporting that SpO2 and HR are not at their "missing data" indicator valued (127 and 511, respectively) and simultaneously reporting that the OOT status flag is false.

Thanks! I have made some changes to Sensor.cpp but it's still WIP and we can discuss it in our meeting tomorrow.

[rohanpurohit]

@ethanjli This pr is ready for testing. after which we can do one final review depending on test results

Tests:

• We should be testing if we get normal output or for some reason, we return failed and display NaN instead.
• We should see if we get Nonin sensor disconnected on disconnecting the sensor from mcu.
• On disconnecting the finger sensor, is there an alarm for finger sensor disconnected.
• If there are alarms for out_of_track and sensor_alarm which are unlikely and mostly when the sensor is incapable of providing good measurements.

anything else you might think of while testing, do let me know! thanks.

[ethanjli]

Here are my results from testing:

• The changes made have not yet fixed the problem. However, they did make it possible for me to better understand what's happening via the STM32 debugger. If I set two breakpoints - one inside the post_setup_reset method, and the other for prev_state_ = InitializableState::failed inside the output method, then I see that I first hit the prev_state_ = InitializableState::failed before I hit the breakpoint inside post_setup_reset. This would suggest that we're calling the output method as part of the main function's Setup loop, i.e. after the Nonin Sensor has completed its setup routine but before the other sensors have completed their setup routines (during which they may sleep for > 2 ms), the Nonin Sensor::output method is being called. It turns out that this is caused by the main function's "Configure the simulators" section, specifically https://github.com/pez-globo/pufferfish-software/blob/10cabafc474ea24cc2d6da6d12a0f698a7a6c1f0/firmware/ventilator-controller-stm32/Core/Src/main.cpp#L621 , being before the "Reset nonin timer" section. If I swap the order of the two sections, then I am able to get some data from the Nonin sensor. The rest of this test report is made after applying this fix.
• Then what I observe is that on the dashboard, with my finger not in the Nonin sensor, SpO2 is displayed as "--" good, while HR is displayed as "127" (this looks suspiciously like a missing data value, except the Nonin datasheet says the HR missing data indicator is 511 while the SpO2 missing data indicator is 127). Once I put my finger in the sensor, after the expected delay I start getting reasonable SpO2 and HR readings (e.g. 98%, 83 BPM). Once I remove my finger from the sensor, after the expected delay the SpO2 goes back to "--" - and then the HR goes back to 127. I'm not sure what's happening here.
• I also did 15 trials of unplugging the Nucleo board from USB and plugging it back in. In 5 of those trials, the Nucleo board failed setup. The seemingly random pattern suggests to me that maybe our measurement timeout is too low for some reason. When I increase the measurement timeout from 2 sec to 5 sec and repeat 25 trials, the Nucleo board fails setup in 0 of those trials. For this PR we should just increase the measurement timeout; however, once you have the Nonin hardware working on your end we'll need to investigate to understand why a Nonin measurement timeout of 2 sec could be causing the setup routine to randomly fail sometimes. It's probably some kind of interaction with the setup methods of the other drivers, so you could begin by trying to combine the Nonin driver with each of the other drivers in the initialization list, one at a time, to see if the random occurrence of Nonin measurement timeouts can be reproduced with just a single other driver.

Conclusion: just a few small changes and I'm pretty confident that it will work enough for us to merge this PR.

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?
2. Were any of these contributions also part of work you did for an employer or a client?
3. Does this work include, or is it based on, any third-party work which you did not create?

[rohanpurohit]

@ethanjli one interesting thing to note is in the previous code that you wrote for Sensor.cpp, for both HR and SpO2 you checked for 127 which is the missing data for SpO2 Sensor.cpp, and it matches to what we are observing, which would definitely fix that bug! but of course, we'll need to figure out why that happens.

I guess this is the code block that's doing that change PacketReceiver L63 and looks like that is what we want since the LSB is for SpO2

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]

TODO for me: test, and see if it's easy to fix the suspicious HR MSB processing at https://github.com/pez-globo/pufferfish-software/blob/95833b1adcf3d9255a406c8b876d4819acc2f9c4/firmware/ventilator-controller-stm32/Core/Inc/Pufferfish/Driver/Serial/Nonin/PacketReceiver.h#L63

TODO for you (this work does not block this PR): investigate whether FrameBuffer should behave more like RingBuffer or more like ChunkSplitter in its interface for reporting input_overwritten (i.e. as a return code or as an output parameter)

[ethanjli]

I've pushed up a commit completing the fix of the HR parsing code, as well as adding definitions to the frontend's logs/constants.ts for handling the new events you added. With these fixes, everything seems to work; there's a very noticeable delay between when I put my finger in the sensor and when the frontend shows numbers, but I haven't checked whether this is the same delay as before or a larger delay.

For records-keeping on my contributions:

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]

We concluded that FrameBuffer shouldn't modify any internal state if it's already full, it should just return early. This is like the interface of RingBuffer. By contrast, ChunkSplitter clears the internal buffer if it's full and another input is overwritten. There it's reasonable because dropping one frame doesn't affect the state of the protocol, since the protocol doesn't really have connection state at the levels above frames. For the Nonin sensor, there is connection state at the packet level, because each packet is a sequence of 25 consecutive frames and dropping one will invalidate the packet anyways - so clearing the internal buffer wouldn't help error recovery.