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.

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

Power on with duration, power on if brightness > 0 #40

Closed
wants to merge 22 commits into from
Closed

Power on with duration, power on if brightness > 0 #40

wants to merge 22 commits into from

Conversation

p1r473
Copy link
Contributor

@p1r473 p1r473 commented Sep 19, 2021

Hi Linus,
On my Pi4, I am able to successfully fade with a duration with rpi-backlight -b 0 -d 3. However, I cant power on/off with a duration rpi-backlight -p off -d 3. I can, however, toggle with a duration just fine rpi-backlight -p toggle -d 3
I believe we should be able to power on/off with a duration based on this line in the code, showing the intention that power setting may indeed be set with a duration.
parser.error("-p/--set-power may only be used with -d/--duration")

After checking the code, I found that powering on/off with a duration wasn't actually implemented yet
So, I added it as a feature:
-Power on/off is now supported with duration

Additionally, I found that if you set the brightness to 100 while the power is off, nothing happens. So, I added an extra 2 features:
-If setting brightness >0, turn the power on first. (This will better the user experience)
-If setting brightness to 0, turn the power off once brightness is 0. (Perhaps $ energy savings?)

I also added a quick check to the code.
-Do not try to power on if already on (This will prevent the brightness from setting to 100 if brightness is already turned up (i.e. 40) and you try to power on)

@linusg
Copy link
Owner

linusg commented Oct 21, 2021

Hi @p1r473 - sorry for ignoring your PR for so long! Totally missed this, only saw it when looking at the other issue earlier.
I'll review this later today, in the future please just ping me if I don't respond :)

@p1r473
Copy link
Contributor Author

p1r473 commented Oct 25, 2021

Ping @linusg

Copy link
Owner

@linusg linusg left a comment

Choose a reason for hiding this comment

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

Sorry, but I don't think this is a good idea...

rpi_backlight/cli.py Outdated Show resolved Hide resolved
rpi_backlight/cli.py Outdated Show resolved Hide resolved
rpi_backlight/cli.py Outdated Show resolved Hide resolved
rpi_backlight/cli.py Outdated Show resolved Hide resolved
rpi_backlight/cli.py Outdated Show resolved Hide resolved
rpi_backlight/cli.py Show resolved Hide resolved
p1r473 added 7 commits July 27, 2022 11:13
added support for Debian Bullseye
add support for bullseye
add support for bullseye
clean up
clean up
blacked
blacked??
@p1r473 p1r473 requested a review from linusg July 27, 2022 16:04
Copy link
Owner

@linusg linusg left a comment

Choose a reason for hiding this comment

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

These code changes no longer belong together, either squash the history into two distinct, clean commits, or make a second PR for the path changes if you can't do that.

rpi_backlight/__init__.py Outdated Show resolved Hide resolved
rpi_backlight/__init__.py Outdated Show resolved Hide resolved
if args.duration:
with backlight.fade(duration=args.duration):
backlight.brightness = 0
backlight.power = False
else:
backlight.power = True if args.set_power == "on" else False
Copy link
Owner

Choose a reason for hiding this comment

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

This branch is now dead code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ive moved the new path stuff to
https://github.com/j-coopz/rpi-backlight
So this branch can still be about the previous issues

Copy link
Owner

Choose a reason for hiding this comment

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

No idea what that means, but please update if you want to get this PR merged

Copy link
Contributor Author

@p1r473 p1r473 Aug 1, 2022

Choose a reason for hiding this comment

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

Hi Linus, could you please elaborate what you want updated in this code?
This new code does the following:
-Power on/off is now supported with duration
-If setting brightness >0, turn the power on first. (This will better the user experience)
-If setting brightness to 0, turn the power off once brightness is 0. (Perhaps $ energy savings?)

rpi_backlight/cli.py Show resolved Hide resolved
@p1r473
Copy link
Contributor Author

p1r473 commented Aug 1, 2022

Ive moved the new path stuff into #50
This PR can remain the old stuff as it has all the context

@p1r473 p1r473 requested a review from linusg August 1, 2022 01:31
Copy link
Contributor

@j-coopz j-coopz left a comment

Choose a reason for hiding this comment

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

Resolved all conversations (for now?) - whoops, this was done from my alternate account.

@p1r473 p1r473 closed this Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants