Closed ethanjli closed 3 years ago
@rohanpurohit This PR is now ready for testing. Since it refactors/touches a lot of modules, it would be helpful if you could exercise as many functionalities of the firmware as possible during testing. I've tagged you and Raavi in the PR description for items where certain changes may impact how you implement things in the firmware.
For records-keeping:
without commenting out the initializables array, ventilation cannot be started and shows that the connection to the MCU is lost, also fixes the bug where it takes straight to dashboard sometimes
Hmm, I'm not sure I understand what you mean. I personally am able to run the firmware and start ventilation with the following in main.cpp
:
auto initializables = PF::Driver::make_initializables(sfm3019_air, sfm3019_o2, /*fdo2,*/ nonin_oem);
(I comment out fdo2
because I don't have a working hardware unit for it). If you comment out the elements in the array for the sensors you're missing, are you able to run the firmware and start ventilation? I would expect that if you comment out the entire line declaring/defining the initializables
object, the firmware shouldn't compile (because main()
uses the variable).
without commenting out the initializables array, ventilation cannot be started and shows that the connection to the MCU is lost, also fixes the bug where it takes straight to dashboard sometimes
Hmm, I'm not sure I understand what you mean. I personally am able to run the firmware and start ventilation with the following in
main.cpp
:auto initializables = PF::Driver::make_initializables(sfm3019_air, sfm3019_o2, /*fdo2,*/ nonin_oem);
Oh! so to clarify i don't have any of those sensors with me currently, i plan to get them soon, so i remember facing a situation where if i DONT
do this
auto initializables = PF::Driver::make_initializables(/*sfm3019_air, sfm3019_o2, fdo2, nonin_oem*/);
it would take me straight to the dashboard instead of what I'm seeing currently where it says lost connection to the mcu, n i see the same on develop so maybe I'm confusing it with some other bug that was not fixed in this PR.
(I comment out
fdo2
because I don't have a working hardware unit for it). If you comment out the elements in the array for the sensors you're missing, are you able to run the firmware and start ventilation?
Yes, I'm able to. sorry for the confusion
Ah, that's good to know. Currently, if the firmware fails setup, the frontend's landing page still allows us to press "Start" and "Start Ventilation", but it also reports that it has lost its connection to the hardware controller; if I press "Start Ventilation", the button doesn't do anything (should be fixed with #361). We should ensure that the microcontroller is connected and active before we enable the "Start" button from the landing page (which requires #371).
This PR fixes #322 by:
Debouncer
class in the firmware fromPufferfish/Driver/Button
toPufferfish/Protocols/Application
.Debouncer
class's member names and fixing errors in the implementation ofDebouncer
.Debouncer
class to the backend, so that the backend can debounce alarms, and making the breathing circuit simulator alarms be debounced.Debouncer::transform
method to fit the signal-processing filter pattern used inPufferfish/Protocols/Application/SignalSmoothing.h
, in which thetransform
method always writes its output signal to thebool &output
parameter (rather than only writing the output signal when the method returnsStatus::ok
).int
before comparing against theint
alarm limits. This makes the alarm triggering behavior less twitchy and more consistent with the values displayed on the frontend. e.g. if the SpO2 lower alarm limit is 40%, an SpO2 measurement of 39.6%, which the frontend displays as 40%, shouldn't trigger the alarm.EnumMap
class for a map withuint*_t
-backedenum
s as keys and arbitrary values, featuringO(1)
insertion,O(1)
lookup and deletion, andO(n)
traversal, withO(n)
memory usage (wheren
is the largest integral value used by the enum), intended for use as a fast lookup table. For comparison,OrderedMap
hasO(m)
insertion,O(m)
lookup and deletion, andO(m)
traversal, withO(m)
memory usage (wherem
is the maximum number of key-value pairs allowed in the map), intended for use in quickly generating small lists of key-value pairs (e.g. the list of active alarms).EnumMap
to store the debouncer for each debounced alarm code.BreathingCircuit/Alarms.h
file to define the list of debouncers for the breathing circuit (and also using that as the list of alarms to deactivate when ventilation stops, instead of having an array of alarm codes inBreathingCircuit/AlarmsService.h
). Currently,Application::AlarmsManager
can only handle a single list of debouncers (i.e. those for the breathing circuit); a future PR will be needed to be able to also define lists of debouncers for other modules (e.g. power management, if debouncing is needed there) and give them to the same instance ofApplication::AlarmsManager
. (FYI @rohanpurohit, in case this is relevant to #327)Util::Timer
class to encapsulate a starting time and timeout together forUtil::within_timeout
calls.Util::Timer
to add a "lock-out" mechanism for alarms in the firmware and backend, so that FiO2 and flow rate alarms won't be triggered in the ~2 seconds afterParametersRequest.fio2
orParametersRequest.flow
are changed, to give the system time to reach the requested values before sounding alarms. Note: this "lock-out" mechanism is only implemented in the firmware, not the backend. We can port it to the backend later if needed.setup
method of theSensor
driver for the Nonin OEM III to have been called at least once before it starts producing measurements (and returningInitializableState::ok
) withoutput
. This allows us to disable the driver for the Nonin OEM III by excluding it frommain.cpp
's list ofInitializable
objects, and thus to enable SpO2 and HR simulation - see the next item.main.cpp
's list ofInitializable
objects). Commenting outsfm3019_air
andsfm3019_o2
is required for the simulator to simulate flow rates, while commenting outnonin_oem
now is required for the simulator to simulate SpO2 and HR. This also fixes a bug where previously the simulator was adding simulated noise to real SpO2 and HR measurements from the Nonin OEM III sensor, which was making the displayed values bounce more than they should. @rohanpurohit If you can follow a similar pattern for choosing when to run the power simulator in #327, that'd be great. Basically, between the setup and the main loop I set some variables from the outcomes of the initialization routine, by trying to calloutput
methods. You can do the same for theltc4015
sensor for abool ltc4015_status
variable, which you can then use as yourif
check for whether to call thepower_simulator.transform
method.This PR also does some refactoring in the firmware:
EnumMap
to store the nanopb message descriptor for each message type used byPufferfish/Protocols/Transport/Messages.h
. This allows us to associate enums directly withMessageTypes
enum values, rather than having to also figure out the array indices corresponding to those enum values and to deal with gaps between enum values (e.g. so that it won't be annoying to have message types with uint values 1-12 and then message types with uint values 64-80 and then message types with uint values 128-135).Util::Timer
instead ofUtil::within_timeout
wherever reasonable, to encapsulate starting times together with timeout durations. (FYI @raavilagoo and @rohanpurohit we should prefer usingUtil::Timer::within_timeout
in the future, to make the code more readable. @rohanpurohit we should switch fromUtil::within_timeout
toUtil::Timer::within_timeout
in #327's power management simulator)Initializable
s into a newInitializables
class inPufferfish/Driver/Initializable.h
.transform
methods so that all input parameters (const references and values) come before all output parameters (non-const references). FYI @rohanpurohit we should make sure we do the same in #327, and anything which can be a const reference should be so (e.g.AlarmsService::transform(PowerManagement &power_management,...
should be changed toAlarmsService::transform(const PowerManagement &power_management,...
)HAL/Platform.h
which is the only file which selects the platform used by the firmware; it's used to set the definitions forHAL/Endian.h
andHAL/Types.h
.Util::Containers::Vector
so that we can iterate throughVector
objects with range-basedfor
loops (FYI @raavilagoo and @rohanpurohit so you can take advantage of this when possible)This PR also reorganizes some files in the firmware:
Pufferfish/Protocols
are moved intoPufferfish/Protocols/Transport
orPufferfish/Protocols/Application
, following the pattern set in the backend'sventserver.protocols
. The unit test files are also moved to match this directory structure.Pufferfish/Util
toPufferfish/Util/Containers
.Due to the file reorganization: