Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Respect StatusLED configuration at ShutdownLED when shutdown sequence is cancelled and the led share the same GPIO #1997

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from
Draft

Respect StatusLED configuration at ShutdownLED when shutdown sequence is cancelled and the led share the same GPIO #1997

wants to merge 7 commits into from

Conversation

topas-rec
Copy link
Contributor

I would like to keep the led on when the shutdown sequence (holding the shutdown button for the "hold time") is cancelled and a StatusLED is configured.
It should be off when the sequence is cancelled and no StatusLED is configured.

@topas-rec topas-rec changed the title Aborting shutdown sequence now checks for StatusLED configuration Respect StatusLED configuration at ShutdownLED when shutdown sequence is cancelled and the led share the same GPIO Mar 18, 2023
@topas-rec topas-rec force-pushed the fix-statusLed-stays-off-when-ShutdownButton-LED-is-activated-and-shutdown-request-is-aborted-by-releasing-the-shutdown-button-before-the-hold-time-expired branch from 6ef60c0 to b63b715 Compare March 18, 2023 08:58
@topas-rec
Copy link
Contributor Author

Yeah, like this so far.
How can this be improved by using object oriented approach?

@topas-rec topas-rec force-pushed the fix-statusLed-stays-off-when-ShutdownButton-LED-is-activated-and-shutdown-request-is-aborted-by-releasing-the-shutdown-button-before-the-hold-time-expired branch from af11c7d to 6af1a76 Compare March 18, 2023 09:28
@s-martin
Copy link
Collaborator

Thanks for this PR! 🎉

could you check the Python unit tests, which are currently failing and try to fix them in this PR?
https://github.com/MiczFlor/RPi-Jukebox-RFID/actions/runs/4454707894/jobs/7824072075?pr=1997

@topas-rec topas-rec marked this pull request as draft March 28, 2023 17:48
@topas-rec
Copy link
Contributor Author

Thanks for this PR! tada

could you check the Python unit tests, which are currently failing and try to fix them in this PR? https://github.com/MiczFlor/RPi-Jukebox-RFID/actions/runs/4454707894/jobs/7824072075?pr=1997

Thanks for looking into it.
I was thinking out loud in this PR so far and this is not complete and I don't expect the checks to pass. I also did not test anything in here.
I converted this to draft now and I hope that the checks are not running any longer.

I created this PR, because I wanted to start with fixing it, though I have no clear way on how to implement it. Doing something sometimes helps rather than thinking days about it.
I appreciate help on how to integrate this fix properly.
If you can, try to understand my thoughts and notes above and look at the current progress. It's not much and the fix is not hard to do, but there are some design decisions to make. I am happy to hear some hints.

I plan to continue this when I find some free time.

@topas-rec topas-rec force-pushed the fix-statusLed-stays-off-when-ShutdownButton-LED-is-activated-and-shutdown-request-is-aborted-by-releasing-the-shutdown-button-before-the-hold-time-expired branch from 6af1a76 to 00a51fd Compare May 10, 2023 18:24
@topas-rec topas-rec force-pushed the fix-statusLed-stays-off-when-ShutdownButton-LED-is-activated-and-shutdown-request-is-aborted-by-releasing-the-shutdown-button-before-the-hold-time-expired branch 2 times, most recently from 8713fe4 to 7dd5364 Compare May 10, 2023 19:30
@topas-rec topas-rec force-pushed the fix-statusLed-stays-off-when-ShutdownButton-LED-is-activated-and-shutdown-request-is-aborted-by-releasing-the-shutdown-button-before-the-hold-time-expired branch 2 times, most recently from ba4558e to 71d05d7 Compare May 20, 2023 06:33
@topas-rec
Copy link
Contributor Author

topas-rec commented May 20, 2023 •
edited
Loading

Tested in unit tests so far and I am happy with it.
Final test with this code on the real hardware has to be done with the following configurations in gpio_settings.ini:

@topas-rec topas-rec marked this pull request as ready for review May 20, 2023 09:37
@topas-rec
Copy link
Contributor Author

Should the test adoptions go into a separate commit? @s-martin ?

@topas-rec
Copy link
Contributor Author

topas-rec commented May 20, 2023 •
edited
Loading

I think the tests never finish because of a while loop never succeeding in on the test systems.
See https://github.com/topas-rec/RPi-Jukebox-RFID/blob/71d05d79c357c5e7663628599bc9b69a04d878b3/components/gpio_control/GPIODevices/led.py#L39

@s-martin
Copy link
Collaborator

I think the tests never finish because of a while loop never succeeding in on the test systems.
See https://github.com/topas-rec/RPi-Jukebox-RFID/blob/71d05d79c357c5e7663628599bc9b69a04d878b3/components/gpio_control/GPIODevices/led.py#L39

Yeah, that could be the issue.

A method to overcome such scenarios could be mocking the methods during the tests.

please have a look here for starters: https://docs.python.org/3/library/unittest.mock-examples.html

@topas-rec
Copy link
Contributor Author

I already mocked it by adding a mock inside that file led.py. Then the test works.
I added it in there because it has an include for os.system which shadows (I think) the mock for the system function from the file that holds the tests .

Mocking inside the file led.py would then be present in production.

@topas-rec topas-rec force-pushed the fix-statusLed-stays-off-when-ShutdownButton-LED-is-activated-and-shutdown-request-is-aborted-by-releasing-the-shutdown-button-before-the-hold-time-expired branch 2 times, most recently from 50e6290 to 71d05d7 Compare May 27, 2023 05:53
topas-rec
topas-rec commented May 27, 2023
View reviewed changes
components/gpio_control/GPIODevices/led.py Outdated
Comment on lines 4 to 6
from mock import MagicMock

system = MagicMock(return_value=0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the test works, but the code doesn't work in production anymore I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't get the test to work when I mock in https://github.com/topas-rec/RPi-Jukebox-RFID/blob/305aa26bec34ca18b8a248d5aff7e502e9a0e7c4/components/gpio_control/test/test_gpio_control.py

@topas-rec topas-rec marked this pull request as draft May 27, 2023 06:01
topas-rec added 4 commits May 27, 2023 20:59
@topas-rec
Made test_gpio_control work again
c4d8aad 
* Test configuration file had still `functionCallDown` and `functionCallUp` as function calls instead of newer `functionCall1` and `functionCall2`
* Made path of config file relative to the location from where the imports work (and the test is executed which is `./components/gpio_control/`
@topas-rec
Mocked function os.system in test_gpio_control
ecf4670 
* Test didn't finished without this setting since os.system is used to check if a phoniebox service is running on the machine which may not run on machines this unit tests run on
@topas-rec
Aborting shutdown sequence now checks for StatusLED configuration
b4bdfe8 
* Code not working
* Base for discussion
@topas-rec
Add dependency check for devices
51364e9
topas-rec added 2 commits May 27, 2023 21:00
@topas-rec
Set the led state for shutdown cancellation in ShutdownButton and use…
fa2fecb 
… this value
@topas-rec
Implemented dependency check between ShutdownButton and StatusLed
e1db039 
* untested
* If a ShutdownButton and a StatusLed are both configured and use the same led pin, the status that the led should have in case a shutdown request (blinking shutdown led) is cancelled is set at the ShutdownButton
@topas-rec topas-rec force-pushed the fix-statusLed-stays-off-when-ShutdownButton-LED-is-activated-and-shutdown-request-is-aborted-by-releasing-the-shutdown-button-before-the-hold-time-expired branch from 305aa26 to 54580f5 Compare May 27, 2023 19:04
@topas-rec
Copy link
Contributor Author

Made mocking work in #2034.
Merge this after #2034.

@topas-rec topas-rec force-pushed the fix-statusLed-stays-off-when-ShutdownButton-LED-is-activated-and-shutdown-request-is-aborted-by-releasing-the-shutdown-button-before-the-hold-time-expired branch from 54580f5 to e51652e Compare May 27, 2023 19:24
@s-martin s-martin added the legacy_v2 Issues, discussions and PRs related to Version 2.x label Dec 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-martin s-martin s-martin left review comments

Assignees
No one assigned
Labels
enhancement legacy_v2 Issues, discussions and PRs related to Version 2.x
Projects
None yet
Milestone
No milestone
2 participants
@topas-rec @s-martin