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

Create 3 `navigate_to` is not using computed timeout #34

Closed scottcandy34 closed 7 months ago

scottcandy34 commented 7 months ago

Found unused and useless line at 162 in create3.py. Became useless after this commit https://github.com/iRobotEducation/irobot-edu-python-sdk/commit/76cd642d48f8463251f8f3962a33d8877d3e7e64

timeout = self.DEFAULT_TIMEOUT + int(math.sqrt(x * x + y * y) / 10) + 4 # 4 is the timeout for a potential rotation.

https://github.com/iRobotEducation/irobot-edu-python-sdk/blob/1a81d6f533937498bee43621b2c10fb2e6f81d1a/irobot_edu_sdk/create3.py#L162C9-L162C17

shamlian commented 7 months ago

You found a bug! I went to all the trouble of computing a timeout and then forgot to use it 🤦

scottcandy34 commented 7 months ago

The timeout that you generate is usually longer than the journey it takes so it hangs if its short. If I traveled for 1 second and then travel back to where I was the timeout is always longer than 1 second. When in reality if it took 1 sec to travel back then it should be completed by the time it gets there which is 1 sec. So the timeout should be 1 sec.

I ran into this problem a year ago. I adjusted it by adding a timeout call to the function and setting that instead. Then I would just pass in 1.0 as the timeout. Not sure on how to get the math correct.

class Create3(Create3): # Modified Create3 Package
    def __init__(self, backend):
        super().__init__(backend)

    async def navigate_to(self, x: Union[int, float], y: Union[int, float], heading: Union[int, float] = None, timeout: Union[int, float] = None):
        """ If heading is None, then it will be ignored, and the robot will arrive to its destination
        pointing towards the direction of the line between the destination and the origin points.
        Units:
            x, y: cm
            heading: deg
        """
        if self._disable_motors:
            return
        dev, cmd, inc = 1, 17, self.inc
        _heading = -1
        if heading is not None:
            _heading = int(heading * 10)
            _heading = bound(_heading, 0, 3599)
        if timeout is None:
            timeout = self.DEFAULT_TIMEOUT + int(math.sqrt(x * x + y * y) / 10) + 4
        payload = pack('>iih', int(x * 10), int(y * 10), _heading)
        completer = Completer()
        self._responses[(dev, cmd, inc)] = completer
        await self._backend.write_packet(Packet(dev, cmd, inc, payload))
        await completer.wait(timeout)
shamlian commented 7 months ago

The timeout should be longer than the journey -- that's why it's a timeout. The completer is waiting for the action to return a result. If the function properly uses the computed timeout, it should work. If the robot is not returning a message after the action completes, that is a bug.

scottcandy34 commented 7 months ago

That's how I would expect it to work.

I wrote a reverse navigation that saves its location every 1 sec then once it is told to stop it will follow along its path backwards. I kept having issues with the timing it wouldn't work properly eventually I just wrote that override above and it fixed my issues.

shamlian commented 7 months ago

I see. I need to investigate what is going on, and that will take some time. Thanks for making me aware of the issue.

scottcandy34 commented 7 months ago

I can share my code with the override for you to use as a testing platform if you wish

shamlian commented 7 months ago

An example describing what you are trying to do, what you think should happen, what actually happens, and the steps you took to try to fix it are always welcome.

It occurs to me that perhaps you are trying to get the robot to use its ROS 2 actions (the BLE interface is just a ROS 2 node under the hood) faster than it is able to respond (especially if you are issuing goals once per second), but perhaps there is something else going on.

scottcandy34 commented 7 months ago

I was just trying to write some cool code within the web python app, that students could use to learn about the robot before ROS2 is taught. I wrote a working wall following program, and thought it would be cool if it could backtrack itself out of a maze without any sensors. So I stored the position every 1 sec interval and basically loop though it and waited for it to complete before sending the next nav.

I honestly barely remember what was happening in detail. As for the steps I backed tracked the code and followed the trail eventually leading to me overriding it completely. The timing was all messed up in my observations. I just did lots of testing. I think it took me a few days to figure out what was happening.

What I think should happen is that it shouldn't take at least a minimum of 7 sec to complete a navigation, if it doesn't send a completed message (now that I think about it that might of been one of the problems too. I know I wrote some test code to output the Bluetooth received data at one point. So I was reading the raw data).

What's funny is I wrote the wall follower in ROS2 just recently and its slower to respond than the web python LOL. I'm certain other variables are at work there. Not important.

Here is my code I uploaded it to my repo Script. For testing have it run and then click the single dot button on the bot to have it backtrack. To see what was going before with the issues just remove the , timeout = nav_save_int on line 244. NOTE I used H.0.0, H.1.0, H.1.1, H.1.2. And the Galactic versions of the same time too.

shamlian commented 7 months ago

Aside: we've made significant strides in robot performance since H.1.2; I'd recommend you try with G.5.4 / H.2.4, if you're interested, but separately from that I plan to look into this system and see what is going on.

scottcandy34 commented 7 months ago

Yes I have my bot on with the H.2.3 already and will tryout the navigation. H.2.4 and G.5.4 is not on the Docs yet. or on the latest-fw links

Much appreciated.

shamlian commented 7 months ago

That's correct, they are not yet officially released. PR here: https://github.com/iRobotEducation/create3_docs/pull/505