EV3-G API infrared sensor by dwalton76 · Pull Request #374 · ev3dev/ev3dev-lang-python

dwalton76 · 2017-09-24T17:13:41Z

No description provided.

dwalton76 · 2017-09-24T17:17:52Z

We have the InfraredSensor functionality split into three classes

InfraredSensor
RemoteControl
BeaconSeeker

but the EV3-G block do all of this via on InfraredSensor block. I like the way we did it much more, it feels much cleaner. Anyway, I moved the functionality from RemoteControl and BeaconSeeker up to InfraredSensor but did so in a way to make RemoteControl and BeaconSeeker backwards compatible.

WasabiFan

Would it be possible to re-arrange the classes as they were before so the diff isn't so weird?

WasabiFan · 2017-09-24T17:33:06Z

ev3dev/core.py

        return value

+    @property
+    def auto_mode(self):


Why? I can't imagine we have anyone using this property... I think we should just remove it.

We don't use it in any of the demos...I would be ok with chopping it

WasabiFan · 2017-09-24T17:35:56Z

ev3dev/core.py

+
        self._mode = self.set_attr_string(self._mode, 'mode', value)
+        self._mode_value = value



Don't you need to update the getter as well?

True I could just have it return self._mode_value

dwalton76 · 2017-09-24T17:44:39Z

Looking into the api_test failure...

ddemidov · 2017-09-24T17:54:10Z

ev3dev/core.py

    '_decimals',
    '_driver_name',
    '_mode',
+    '_mode_value',


The PR title says 'Infrared sensor', but this looks like a move to introduce caching for the mode value. Should this be split into a couple of PRs?

Will pull this part out into a new PR

dwalton76 · 2017-09-25T00:29:40Z

Would it be possible to re-arrange the classes as they were before so the diff isn't so weird?

yep will put them back where they were, we can move them around later via another PR

dwalton76 · 2017-09-25T00:44:42Z

Should be easier to read now

WasabiFan · 2017-09-25T00:56:46Z

ev3dev/core.py


-class InfraredSensor(Sensor):

+class InfraredSensor(Sensor):


Should I be worried?

I had to look at this for a minute to figure out what in the world this change was. It is because I deleted the blank line between the class InfraredSensor and the """. It is odd that it displayed it that way though

WasabiFan · 2017-09-25T00:57:31Z

ev3dev/core.py

-      'IR-REMOTE',
-      'IR-REM-A',
-      'IR-CAL',
+      MODE_IR_PROX,


WasabiFan · 2017-09-25T00:57:51Z

ev3dev/core.py

    )

+    _BUTTON_VALUES = {
+            0: [],


Could we get a comment explaining what these indicies are?

How about

The following are all of the various combinations of button presses for the remote control. The key/index is the number that will be written in the attribute file to indicate what combination of buttons are currently pressed.

Looks good!

WasabiFan · 2017-09-25T00:58:29Z

ev3dev/core.py

+            }
+
+    #: Handles ``Red Up`` events.
+    on_red_up = None


Are the buttons color-coded? I have never actually used the remote after taking it out of the box 😆

They are all gray but there is a red mark on the remote between the two buttons on the left and a blue mark between the two on the right.

WasabiFan · 2017-09-25T00:59:32Z

ev3dev/core.py

-        if self.auto_mode:
-            self.mode = self.MODE_IR_PROX
+    # BeaconSeeker
+    def heading(self, channel=1):


What's the idea behind making these methods instead of property getters?

It is so you can pass the channel that you want...the EV3-G block has this.

Oh, duh. Makes sense.

WasabiFan · 2017-09-25T00:59:53Z

ev3dev/core.py

+        Returns heading (-25, 25) to the beacon on the given channel.
+        """
+        self.mode = self.MODE_IR_SEEK
+        channel = max(1, min(4, channel)) - 1


Warn/error if the channel is out-of-bounds?

yeah I'll put an assert in here...it would be really confusing behavior if someone passed in channel 5

WasabiFan · 2017-09-25T01:01:06Z

ev3dev/core.py

+    def distance(self, channel=1):
+        """
+        Returns distance (0, 100) to the beacon on the given channel.
+        Returns -128 when beacon is not found.


Well duh, of course it returns -128 😆 Do we want to catch that and return None instead? I think no one actually reads our docs 😉

Not sure...what does the EV3-G block do in this scenario?

Returning 'None' makes sense.

WasabiFan · 2017-09-25T01:02:05Z

ev3dev/core.py

+        return (self.heading(channel), self.distance(channel))
+
+    # RemoteControl
+    def red_up(self, channel=1):


is_red_up_pressed? or red_up_pressed? I originally read this thinking that this was asking whether "red" was released.

yeah the up/down naming convention is a little odd. I don't think we should change this though, we use the RemoteControl a fair amount in the demos, I imagine these are used a lot.

Now that we are not worried about being backwards compatible do we want to change the names of red_up, red_down, etc? IMO they are pretty confusing. Brainstorming....

We could label them top_left, bottom_left, top_right, bottom_right

We could rename red_up() to is_top_left_pressed(), etc

We could rename on_red_up to on_top_left_press, etc

WasabiFan · 2017-09-25T01:04:40Z

ev3dev/core.py

-
-    #: Handles ``Beacon`` events.
-    on_beacon = None
+    def __init__(self, sensor=None, channel=1):


Is this the order we want it in? Or should channel be first? I'd imagine that providing a custom sensor will be quite rare. Not that it really matters as you can reference it by name anyway.

It was that way before so we should probably leave it as-is

The PR makes the class a subclass of InfraredSensor. Why do we need the sensor parameter (and _sensor field) here at all?

for backwards compatibility

I mean, the BeaconSeeker class here already is InfraredSensor (since it inherits from it). So why store another instance of InfraredSensor in _sensor?

👍 I support this

python zen: "flat is better than nested"

having a single InfraredSensor class sounds like a good call.

We should start keeping track of all breaking changes; do you guys want to do that in the changelog file as we go or add a "changelog" section to the README? Or maybe create an issue thread to post into? I'm up for whatever's easiest for you guys.

We can start a draft of new release here. It seems README could grow too long too fast if we start putting changes there. Another idea is we could add another section in the documentation for this.

Interesting idea! I just created a release where we can try out tracking this; as we merge PRs we should make sure to update it. If this doesn't go well we can try out a more conventional file approach. https://github.com/rhempel/ev3dev-lang-python/releases/tag/untagged-f43144149d22a7d3cc09

Note that the link might break once we add a version tag.

ddemidov · 2017-09-25T06:28:39Z

ev3dev/core.py

+    on_red_up = None
+
+    #: Handles ``Red Down`` events.
+    on_red_down = None


These are user-defined event handling functions that should fire up during call to process() method (and by the way, I don't see the method here). As in

def beep_when_pressed(state): if state: Sound.beep() rc.on_red_up = beep_when_pressed while True: rc.process()

I don't see how these would work as is, since we are merging RemoteControl with the InfraredSensor, and there is no way to set the channel here (we used to set channel in the constructor of RemoteControl).

We should probably change these into dictionaries: on_press[1] = beep_when_pressed (not lists because channel numbers start at 1).

Re process() method: in the RemoteControl class it was inherited from ButtonBase. If we do want to merge RemoteControl with InfraredSensor, we should probably inherit from ButtonBase as well.

gotcha, will test/work on this part this evening

This is working now

How exactly does it work now? Where do we specify channel to use with the handles?

Prior to this PR you would need to create a RemoteControl object for each channel that you wished to define handlers for.

Yes, that is correct. But the new implementation of infrared sensor allows to (or claims to) be channel-independent: channel is passed to methods like heading() or process() instead of being assigned at construction. But for RC handlers to work with different channels users would have to create an instance of InfraredSensor per channel, which does not seem logical. I think we could drop ButtonBase from parents, and reimplement the functionality so that handlers would take not just state, but also channel, or something to the same effect?

If we think that supporting per-channel handlers in a single object is something folks will use

I used this ability for the big shooting robot (EV3STORM?). One channel for leg movement, the other for shooting. It does come handy from time to time.

Good point. Are you ok if we merge this PR and file a new issue to add per-channel capabilities to the handlers? We are at 10 commits and 60+ comments on this PR and short of the per-channel handlers I think it is in pretty good shape.

Let's do that, it seems in good shape otherwise.

Filed #380 for this part

ddemidov · 2017-09-25T06:34:20Z

ev3dev/core.py



-class RemoteControl(ButtonBase):
+class RemoteControl(ButtonBase, InfraredSensor):


I think we should either drop RemoteControl and BeakonSeeker or keep everything as is. The classes do not do anything that InfraredSensor already does not, so why keep them?.

agreed they aren't doing much but we need them for backwards compatibility

ddemidov · 2017-09-26T06:19:44Z

Now that we are not worried about being backwards compatible do we want to change the names of red_up, red_down, etc? IMO they are pretty confusing. Brainstorming....

I like left_top or red_top, side (left/red or right/blue) in front. I see the side being more important, because there is builtin color coding, and the whole side (red/left or blue/right) is usually dedicated to a motor.

dwalton76 · 2017-09-26T12:04:49Z

What do you guys say we go ahead and merge this one and we can kick around renaming red_up, etc via another PR? If we rename them we will only be able to do so in certain parts of the code because the kernel will still use red_up, red_down etc. I'm worried we might make things more confusing...I didn't think about this until after we had already started kicking around names though that is why I didn't bring it up earlier.

Thank you for all of the feedback on this PR, I think this is the most comments I've seen on a single PR here :) I like where we ended up, it looks much cleaner now and is 1:1 with the EV3-G block.

WasabiFan

Remember to add this to the draft release for VNext.

dwalton76 added 3 commits September 23, 2017 12:42

Move InfraredSensor related classes to same area in the file

10782e1

Move ButtonBase above InfraredSensor...RemoteControll needs it

7444513

Implement EV3-G API in InfraredSensor class

d168d06

WasabiFan reviewed Sep 24, 2017

View reviewed changes

ddemidov reviewed Sep 24, 2017

View reviewed changes

Fix for api_test failure

86bb580

undo 'Move InfraredSensor related classes to same area in the file'

85d4731

WasabiFan reviewed Sep 25, 2017

View reviewed changes

assert that channel is 1,2,3 or 4

0bf257d

ddemidov reviewed Sep 25, 2017

View reviewed changes

dwalton76 added 4 commits September 25, 2017 12:05

Return None instead of -128 if no beacon found

b12ba1a

Remove RemoteControl and BeaconSeeker

6cc02b3

Move ButtonBase to before InfraredSensor

68bc381

Fix process() for InfraredSensor

9b380c1

ddemidov approved these changes Sep 26, 2017

View reviewed changes

WasabiFan approved these changes Sep 26, 2017

View reviewed changes

dwalton76 changed the title ~~Ev3 api infrared sensor~~ EV3-G API infrared sensor Sep 26, 2017

dwalton76 merged commit 390da18 into ev3dev:develop Sep 26, 2017

This was referenced Sep 26, 2017

InfraredSensor - add support for on_red_up, on_red_down, etc bindings per channel #380

Closed

EV3-G API - breaking changes #381

Closed

ddemidov mentioned this pull request Sep 30, 2017

InfraredSensor on_red_up, etc support for multiple channels #388

Merged


		self._mode = self.set_attr_string(self._mode, 'mode', value)
		self._mode_value = value



		class RemoteControl(ButtonBase):
		class RemoteControl(ButtonBase, InfraredSensor):

Conversation

dwalton76 commented Sep 24, 2017

Uh oh!

dwalton76 commented Sep 24, 2017

Uh oh!

WasabiFan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwalton76 commented Sep 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwalton76 commented Sep 25, 2017

Uh oh!

dwalton76 commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwalton76 Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwalton76 Sep 25, 2017 •

edited

Loading