Closed rohanpurohit closed 3 years ago
@ethanjli I remember we had discussed to keep the alarm mute service in the backend simulator similar to that in the firmware, but it seems like I'm not able to get that working. the pushed file in this branch follows the same logic of having a service like other files and that does work, But I guess mute shouldn't really depend on active service? can you point out problems if any in the first approach? changes:
Your alarm_mute.Service has an active_alarm_ids
member which is never changed from its initial value. I would suggest making active_alarm_ids
be an input parameter of alarmMute.Service.transform
, rather than having it as a member. Right now I would expect the behavior to be that the backend ignores any AlarmMuteRequest
.
This pull request introduces 1 alert when merging a31f0573417ee235381bc03388421705fec0fca5 into f24b7b7b97653f909f71ca3be2fa3130eac96e60 - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 453121969302fc7247f5545c1efed4804e3ed980 into f24b7b7b97653f909f71ca3be2fa3130eac96e60 - view on LGTM.com
new alerts:
@ethanjli I haven't tested the firmware side yet, but I have updated the PR description to state the changes to Audio and alarm mute button behavior, and also muting backend connection lost alarm in frontend. I have also updated the backend simulator (for code-review), If it's possible maybe you can test these changes with the simulator? thanks!
TODO from me before merging: test this branch on the Raspberry Pi with the simulator backend TODO from me: also test this branch on my computer with the firmware
This pull request introduces 1 alert when merging 860018aeab12d187befd975eade1ca4491d74594 into 05e1e75a968ddc8ce06365f9b030d9b8ffce82df - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 3435c0757b92489a4ace268858239a92d7a3e22a into 05e1e75a968ddc8ce06365f9b030d9b8ffce82df - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging baf3c30998892be51b5ac64243637dba022ed0fb into 05e1e75a968ddc8ce06365f9b030d9b8ffce82df - view on LGTM.com
new alerts:
Observations from testing:
AlarmMuteService
hasn't been fixed yet.This pull request introduces 1 alert when merging 690a90cd9ab568b7b43912b093900110478c3da1 into 05e1e75a968ddc8ce06365f9b030d9b8ffce82df - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 304b638751758a1a6d62d62ea2b69061934f7ade into 05e1e75a968ddc8ce06365f9b030d9b8ffce82df - view on LGTM.com
new alerts:
- On the Raspberry Pi, muting behavior works as expected in the simulator and frontend (I could only confirm this the visual border, because for some reason my RPi isn't playing any audio at all, even from other websites like YouTube).
same here. maybe we need to connect a Bluetooth speaker and test it? I don't have one currently so couldn't do that.
- On my computer, muting behavior does not work in the firmware with the frontend. I think this is because the implementation of
AlarmMuteService
hasn't been fixed yet.
yes I have fixed it now and visually it works, also confirmed with the redux store and printing alarm_mute from the mcu.receive event. can you also test the same @ethanjli thanks!
- In either this PR or a future PR (I would suggest a future PR), the AlarmMuteService in the simulator backend and in the firmware, as well as whatever mechanism allows muting in the frontend while the backend is disconnected, will need to update a countdown timer (counting down from 2 min) which gets displayed in the frontend where now it always says "2:00" , and the service implementations will need to set alarmMute.active back to false after the two minutes have elapsed.
oh okay! I will do this in a future PR, just to note after fixing the firmware part, it shows "0:00" instead (only when backend is connected to application, "2:00" on the simulator") haven't looked at why just yet. just writing it down here for future reference.
- In a future PR, we may need to disable the mute button while the firmware is disconnected, to prevent weird behavior.
Yes, I saw this in your comment on the issue. I guess we can keep that issue open and note these down there?
This pull request introduces 1 alert when merging 5f74c2202d11599a98e7359263895e708f9a8214 into 05e1e75a968ddc8ce06365f9b030d9b8ffce82df - view on LGTM.com
new alerts:
I can also toggle alarm muting with the firmware. I suspect that the "countdown time" for the firmware's copy of the AlarmMute
message has not been initialized to 120 seconds, so the firmware makes the frontend that the countdown is 00:00; by contrast, the simulator backend initializes AlarmMute.remaining
as 120. I recommend that you initialize AlarmMuteRequest
and AlarmMute
in the initialize_states
function in the firmware's main.cpp
, following the same pattern used for AlarmLimits
.
This pull request introduces 1 alert when merging 7b404839c4c2e5b1bfd849d671c5b339d6029f80 into 240885fc37fda7f61389c4277a4ac56d4932f161 - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging 431968cc7cc8370b37bb500deb492fba8d1cbe6e into 240885fc37fda7f61389c4277a4ac56d4932f161 - view on LGTM.com
new alerts:
This pull request introduces 1 alert when merging cd93a41578232036b39c498ef2367b09d4982159 into 240885fc37fda7f61389c4277a4ac56d4932f161 - view on LGTM.com
new alerts:
This PR includes:
As stated in #346 there are few issues with mute button behavior on develop (when alarm muting worked) :
This PR fixes above issues:
playing
to play/pause the audiogetBackendConnected
selector and theAlarmMuteRequest
, which is still dispatched even thought there is no backend to forward it to toggle playing the sound.