Skip to content

Conversation

@ddemidov
Copy link
Member

@ddemidov ddemidov commented Nov 7, 2015

This is based on discussion at #37 (comment).

This introduces concepts of led groups (see also ev3dev/ev3dev#412) and colors into Leds class. The led groups may be operated as in:

# Set group color:
Leds.set_color(Leds.LEFT, Leds.AMBER)
Leds.set_color(Leds.RIGHT, (1.0, 0.75))

# Batch set arbitrary led properties in group:
Leds.set(Leds.LEFT, trigger='timer', brightness_pct=0.5)

I hope this is close to what @EricPobot had in mind.

This is accompanied by rhempel/ev3dev-lang@28361da

@ddemidov
Copy link
Member Author

ddemidov commented Nov 7, 2015

Also, I hope @ensonic is ok with this change.

@ddemidov
Copy link
Member Author

ddemidov commented Nov 7, 2015

rhempel/ev3dev-lang#5, if/when merged into ev3dev/ev3dev-lang, will break js and C++ bindings, so pinging @WasabiFan as well.

@EricPobot
Copy link
Contributor

It seems to do the job as expected. OK for me.

BTW dealing with the colours rather that with the individual leds avoids a logical but disturbing behaviour which made me scratch my head recently. While the leds are on in amber (or any mix), red_on() and alike seems to be inoperative. In fact they are, but have no visual effect, since it is also required to invoke green_off() to obtain the desired result. Once again, what happens is perfectly correct, but not intuitive.
Maybe we should drop individual coloured leds control, and propose only colour setting as you've implemented.

@rhempel
Copy link
Member

rhempel commented Nov 7, 2015

Welcome to the world of hair splitting when it comes to bicoloured LED control. I can give one good reason for retaining individual control, and that is to multiplex more visual information on one LED - ie set one background color for system state, but still allow flashing for disk access. Maybe that's in the realm of overthinking it..

@ddemidov
Copy link
Member Author

ddemidov commented Nov 7, 2015

Leds class exposes all the leds present in the system individually as well as in groups. I think that should be enough for any use case.

@EricPobot
Copy link
Contributor

@ddemidov I'm OK with your proposal. It will always be possible to add a layer on top of this to address specific behaviours.

@ddemidov ddemidov mentioned this pull request Nov 7, 2015
rhempel added a commit that referenced this pull request Nov 7, 2015
Switch to group-based interface in Leds
@rhempel rhempel merged commit 38eec58 into ev3dev:master Nov 7, 2015
@ddemidov ddemidov deleted the leds-group-iface branch November 7, 2015 15:45
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