iRobotEducation / irobot-edu-python-sdk

Python SDK for iRobot Edu robots (Root or Create 3)
BSD 3-Clause "New" or "Revised" License
16 stars 6 forks source link

Bumper sensor not picking up front bumper trigger #10

Closed scottcandy34 closed 8 months ago

scottcandy34 commented 1 year ago

I have create3 robot Firmware version (G.3.1) and have found that you can't identify if the front bumper sensor gets triggered even when the Bluetooth documentation covers it here https://github.com/iRobotEducation/root-robot-ble-protocol#command-0---bumper-event

The event bellow will always trigger which is undesired its no different from using [] to trigger on any event

@event(robot.when_bumped, [true, true])

I have fixed this issue by using pythons override of the create3 class. here is my code

class Create3(Create3):
    async def _when_bumped_handler(self, packet):
            for event in self._when_bumped:
                if len(packet.payload) > 4:
                    self.bumpers.left = packet.payload[4] & 0x80 != 0
                    self.bumpers.right = packet.payload[4] & 0x40 != 0

                    # An empty condition list means to trigger the event on every occurrence.
                    if (not event.condition and self.bumpers.left) or (not event.condition and self.bumpers.right):  # Any.
                        await event.run(self)
                        continue
                    if len(event.condition) > 1 and ((event.condition[0] == self.bumpers.left) and (event.condition[1] == self.bumpers.right)):
                        await event.run(self)

I only changed one line from this if len(event.condition) > 1 and ((event.condition[0] and self.bumpers.left) or (event.condition[1] and self.bumpers.right)):

to this if len(event.condition) > 1 and ((event.condition[0] == self.bumpers.left) and (event.condition[1] == self.bumpers.right)):

I changed the and to an == because this would verify that its equal since the list[bool, bool] is technically matching it so it should be matched. And changed the or to and.

This fixed the issue and allows the front bumper sensor to be accessible in the code by [true, true]

Can this be implemented so I or others don't have to do a class override.

shamlian commented 1 year ago

Thanks for the report and potential fix. Feel free to submit a PR if you're comfortable; if not, I can make one. Let me know!

scottcandy34 commented 1 year ago

no prob. if you would, I currently do not have git installed

shamlian commented 1 year ago

I'm starting to look into this, and it appears that, while there may be a bug in the event not triggering, it is intentional that @event(robot.when_bumped, [True, True]) should trigger the same as @event(robot.when_bumped, []) -- if you look at code.irobot.com level 3, these are annotated as "any bumper pressed" not "both bumper pressed." Indeed, in hardware, both the Create 3 and Root robots only have two bumper switches (left and right). Can you show me the behavior you're trying to build?

shamlian commented 1 year ago

(And as far as -- should we change this design? Maybe. We'll talk about it internally.)

scottcandy34 commented 1 year ago

Ah I understand. I work at a college and have been building a lab that's using the create3. The professor wanted the students to get familiar with them, by turning left when bumped left and turning right when bumped right. Then stop when bumped center. and a few other challenges that used the bumpers.

I just looked at code.irobot.com level 3. That is rather interesting. (first I didn't know you could do that on code. and hadn't realized that this package was used there). Is it possible to just add another option for if both are pressed? I mean it is in the Bluetooth code as an option that both are pressed.

As is the current code if you bumped center it has the potential to trigger both @event(robot.when_bumped, [True, False]) and @event(robot.when_bumped, [False, True]) at the same time, or one before the other because of how python async works.

I understand what the logic is for how it is now but from a programing perspective its strange that its not based off of whether it matches.

miniBloq commented 1 year ago

Hi! Thanks reaching us about this.

As @shamlian explained, the empty event [] for the bumpers is a shortcut for [True, True]. This (and other events) in our system behave like an OR, not like an AND. This was not decided quickly, nor by one single person. It comes from our research with iRobot Coding, where very young students (even non readers) were playing with Level 1. One of the SDK's goals is to keep 100% compatibility with that environment, so students reaching L3 can easily switch to the SDK and/or our Python Web Playground (https://python.irobot.com/).

We decided not to add more complex events, since the SDK already provides some getters (and we will add more in the future), which allow to work with the event data in a more fine grained way. That is why each event passes robot as a parameter. That way, even if there is not a global reference to the robot instance (not having these global references may be the right thing in some non-trivial programs), each event provides the full state of the robot when it was triggered. Here is some code that will do what you want, still using bumper events, but then relying in the getters to achieve the AND function (my current understanding is that that AND function is what you would like to perform -please correct me if I am wrong-):

#
# Licensed under 3-Clause BSD license available in the License file. Copyright (c) 2021-2022 iRobot Corporation. All rights reserved.
#

from irobot_edu_sdk.backend.bluetooth import Bluetooth
from irobot_edu_sdk.robots import event, hand_over, Color, Robot, Root, Create3
from irobot_edu_sdk.music import Note

robot = Root(Bluetooth())
#robot = Create3(Bluetooth())

# Shortuct for @event(robot.when_bumped, [True, True])
@event(robot.when_bumped, []) 
async def bumped(robot): # Triggers when there is a bumper change.
    # More logic can be added to this event in order to prevent triggering when releasing bumpers.
    if robot.bumpers.left and robot.bumpers.right:
        await robot.set_lights_rgb(255, 0, 0)
    elif robot.bumpers.right:
        await robot.set_lights_rgb(0, 255, 0)
    else:
        await robot.set_lights_rgb(0, 0, 255)

@event(robot.when_bumped, [])
async def music(robot):
    # This function will not be called again, since it never finishes.
    # Only tasks that are not currently running can be triggered.
    print('music!')
    while True:
        # No need of calling "await hand_over()" in this infinite loop, because robot methods are all called with await.
        await robot.play_note(Note.A4, .1)
        await robot.wait(0.3)
        await robot.play_note(Note.A5, .1)
        await robot.wait(0.3)

@event(robot.when_play)
async def play(robot):
    print('Hello!')

robot.play()

Small note: I tested this with an rt0 robot, but should work with Create 3 and rt1 as well.

scottcandy34 commented 1 year ago

Thanks, I like your explanation. I will go ahead an use this instead. Do you want me to close this issue?

miniBloq commented 1 year ago

Please do not close it yet. We are having some internal conversation about a potential bug on the bumper events. If there is a bug or some change, this issue will be a good place to let you know. Thanks!

shamlian commented 1 year ago

I updated examples which use [] to fully specify the sensitivity list/filter (that is, to say [True, True] or similar) in the pre_0.2.0 branch. There may still be a bug, so we should keep this open.

shamlian commented 8 months ago

Agreed that this is working as designed.

shamlian commented 7 months ago

moved debate about how events should work to #40