Closed rohanpurohit closed 3 years ago
From an overview-level code review, this looks good. We discussed moving alarms-related functionality out of the high-level Sensor class for LTC4015, and changing the function interface and name to read_charging_status
and renaming the BatteryPower
message field from charging_status
to charging
. The next step is to make any necessary fixes once hardware gets tested, then I'll do another quick review and we can merge.
Changes:
This pull request introduces 1 alert when merging 505d1cd48b4c40a8a0d8512dbef7c245033c6e67 into 240885fc37fda7f61389c4277a4ac56d4932f161 - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging e9d736030fdeb76827961c81f28951a8315f1a14 into b42080c2035f411967bb78b43fea8dcf615b83d6 - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 2abfd333f36431c6dce6ad14f508953f3a139cd6 into 247e71e451a1170639bf2524754e71f712156c32 - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging c3d4622fd2a57a3678142a1502c4a9a4b798ea31 into 3888ca52f4c3a03652588add8eb5b9c905824b4c - view on LGTM.com
new alerts:
Results from testing on commit 875b8ed, with ltc4015_sensor
removed from the list of firmware initializables:
if (!parameters.ventilating)
branch from Pufferfish::Driver::Power::transform
, everything still seems to behave correctly. I don't see any spamming of the event log; and I get this alarm on the quickstart screen, while ventilation is inactive. The only weird behavior I see here is that I get the "Charger is disconnected" event twice in the event log, occurring 23 seconds apart.Results from testing on commit 352f3a3, with ltc4015_sensor
removed from the list of firmware initializables:
if (!parameters.ventilating)
branch from Pufferfish::Driver::Power::transform
(and delete all code in deactivate_alarms
, to make the commit compile), the event log is not spammed with "Charger is disconnected" events.Results from testing on commit 2abfd33, with ltc4015_sensor
removed from the list of firmware initializables:
false
. I got a single alarm that the battery charger is disconnected, but the event log wasn't spammed by that alarm.if (!parameters.ventilating)
branch from Pufferfish::Driver::Power::transform
, the event log is spammed with "Charger is disconnected" events, but only when the ventilator is not ventilating; once the ventilator starts ventilating, it works fineResults from testing on commit 01948ce, with ltc4015_sensor
removed from the list of firmware initializables:
if (!parameters.ventilating)
branch in Pufferfish::Driver::Power::transform
. Battery level is always at 100%, and the event log is spammed with "Charger is disconnected" events.Thus, some change made on 352f3a3 fixed the problem of spamming the event log even without the if (!parameters.ventilating)
branch. My hypothesis is that BreathingCircuit::AlarmsService::deactivate_alarms
was always deactivating the LogEventCode::charger_disconnected
alarm whenever the breathing circuit found that the parameters had set ventilating
to false. Thus, the changes made in 2abfd33 to remove charger_disconnected
from the list of alarms which the Breathing Circuit alarms service could deactivate had a side effect of also solving the root cause of the event log spamming issue. By adding that event code back to the list of alarm codes for the Breathing Circuit alarm service, I was able to bring the spamming back. This also explains why your workaround to disable the charger_disconnected
alarm was able to stop the spamming: then your Power alarms service was no longer fighting against the Breathing Circuit alarms service about whether to activate or deactivate charger_disconnected
.
Since we have identified the root cause of the event log being spammed by charger_disconnected
events, it is now correct to remove the if (!parameters.ventilating)
branch from Pufferfish::Driver::Power::transform
.
This pull request introduces 1 alert when merging 875b8ed3f7fa8974424d5b3667e01a8a9d599df0 into 3888ca52f4c3a03652588add8eb5b9c905824b4c - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging a8550c0d87f46aa85e5bbb8351c32cb4d6d0974a into a65d6afbb0e2759d2f105703df18563ea275e5cf - view on LGTM.com
new alerts:
Results from testing the power simulator in the firmware: simulated charging happens slowly (I think it's a reasonable charging rate), while discharging happens way too quickly (i.e. it goes from 100% to 0% in a few seconds). This bug only occurs in the firmware's power simulator, not in the simulator backend.
This pull request introduces 1 alert when merging c918d2d15a5766a4d0cf8c39603f4afccdd5ce8d into 1f7e1926a3dd0b4656c5efcc64e6fad36cf86b52 - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 2daf69d23ba401742494ed594def51f824f85311 into 1f7e1926a3dd0b4656c5efcc64e6fad36cf86b52 - view on LGTM.com
new alerts:
@ethanjli I have updated the power simulator and higher-level driver, this is ready for testing and code review, I will update the changes you suggested in #378 once we test and merge that.
This pull request introduces 1 alert when merging 8cfa61d336cba4b981bb64658a3e276e2b54b6aa into 1f7e1926a3dd0b4656c5efcc64e6fad36cf86b52 - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 28a89a67037956b12624c3b809eab95764f199b1 into 1f7e1926a3dd0b4656c5efcc64e6fad36cf86b52 - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 8126d84ed9cd61697f8e1856609b052d05bba16c into 1f7e1926a3dd0b4656c5efcc64e6fad36cf86b52 - view on LGTM.com
new alerts:
Here's my review, just minor formatting changes. I tested the backend simulator and firmware simulator and they both work fine now. If we merge this into develop ahead of #378, I'll resolve any merge conflicts and make any changes in #378.
either way is fine with me, I guess we need to use Util::TImer
in the simulator and remove Util::UnrecognizedMessage
as now it uses EnumMap.
This pull request introduces 1 alert when merging 7f4a5aaa6a599a3b06691b15fb7ca2d0895878cd into 1f7e1926a3dd0b4656c5efcc64e6fad36cf86b52 - view on LGTM.com
new alerts:
I just merged #378 into develop before I saw your latest comment. One other change which was buried in that PR's description is that we should change AlarmsService::transform(PowerManagement &power_management,...
to AlarmsService::transform(const PowerManagement &power_management,...
if that alarms service only needs to read values from power_management
and doesn't need to write values to it.
This pull request introduces 1 alert when merging 49a204dd207db9c72d5d9c567d5ad0864dd41af4 into d386a033885c6d89196643199d5f374c170ebfe7 - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 6ec5c16cc4bcc63b61cb09a4b3a106b03c03cf0e into d386a033885c6d89196643199d5f374c170ebfe7 - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 99b29d880566e689dcc16ca33cb251a5155b6f41 into d386a033885c6d89196643199d5f374c170ebfe7 - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging fc176801afb2e1fbd88596c50cd6933363040592 into d386a033885c6d89196643199d5f374c170ebfe7 - view on LGTM.com
new alerts:
This PR includes: