InfraredSensor on_red_up, etc support for multiple channels#388
InfraredSensor on_red_up, etc support for multiple channels#388dwalton76 merged 5 commits intoev3dev:developfrom dwalton76:ev3g-api-infrared-multi-channel
Conversation
|
|
||
| # See process() for an explanation on how to use these | ||
| #: Handles ``Red Up``, etc events on channel 1 | ||
| on_red_up1 = None |
There was a problem hiding this comment.
Would channel1_red_up (C1_red_up, ch1_red_up) be easier to understand? Also, would now be a good time to follow up on #374 (comment) and change red_up to top_red or top_left?
There was a problem hiding this comment.
@dlech wouldn't you also need to rename the buttons in the kernel too?
Regarding the on on_red_up1 I think maybe we look at making these normal methods for the InfraredSensor class and then in the documents show how to make a child class where you override those methods. To me that would be far easier to follow, the current approach is pretty unique and isn't something most folks have seen. I like the channel1_red_up model for naming.
There was a problem hiding this comment.
wouldn't you also need to rename the buttons in the kernel too?
If these names are in the kernel, it is only in the documentation. What you call them in python doesn't depend on what the kernel calls them.
There was a problem hiding this comment.
ah, I see now.
changing red_up to either top_left or top_red sounds fine, of the two top_left would be my preference.
There was a problem hiding this comment.
Having pressed in the name would be confusing though because the button may not be pressed. In red_up that doesn't mean the button is up, it is referring to the upper button on the red side.
Let's do this, I'll commit a patch with top_left, channel1_top_left, etc and we can see what it looks like and then go from there.
|
|
||
| # See process() for an explanation on how to use these | ||
| #: Handles ``Red Up``, etc events on channel 1 | ||
| channel1_top_left = None |
There was a problem hiding this comment.
I just noticed that this drops on_ prefix from the handlers. I think its ok given that the names differ from top_left. top_right, ... properties, and are already quite verbose with channel*_ prefixes. Just wanted to make sure this is intentional. It could be confusing for users (e.g. channel1_top_left could be easily misunderstood to be a boolean property for top left button on channel 1).
There was a problem hiding this comment.
I'll put the on_ back that way the format is consistent with the on_left, on_right, etc for the buttons
|
I think we are good on this one...merging |
For issue #380
@ddemidov look good for EVERSTORM?