Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix loss of steering in path_follow template #1033

Merged
merged 7 commits into from Aug 9, 2022
Merged

Conversation

Ezward
Copy link
Contributor

@Ezward Ezward commented Aug 4, 2022

When using the path_follow template with GPS I would often lose all steering control. I had occasionally seen this in the standard complete.py template as well. It turns out that the steering part would throw an exception on an out of range value and would kill the steering thread. This PR fixes that problem.

  • the CTE PID algorithm in path follow was predicting steering
    values that were way out of the -1 to 1 range.
  • This triggered a check that threw an exception.
  • Since the part was being run in a thread, throwing the exception
    kill the thread, so so the user would lose all steering control.
  • Even the auto-pilot would lose steering.
  • Part 1 of fix is to not throw an exception, but instead log
    and error and return.
  • Part 2 of the fix is to clamp all out of range values so
    that if the PID predicted an out of range value, it would be
    brought back into range.

- the CTE PID algorithm in path follow was predicting steering
  values that were way out of the -1 to 1 range.
- This triggered a check that threw an exception.
- Since the part was being run in a thread, throwing the exception
  kill the thread, so so the user would lose all steering control.
- Even the auto-pilot would lose steering.
- Part 1 of fix is to not throw an exception, but instead log
  and error and return.
- Part 2 of the fix is to clamp all out of range values so
  that if the PID predicted an out of range value, it would be
  brought back into range.
@Ezward Ezward requested a review from DocGarbanzo August 4, 2022 04:12
Copy link
Contributor

@DocGarbanzo DocGarbanzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also bump the version in setup.py

@@ -10,6 +10,18 @@
from typing import Tuple

import donkeycar as dk
from donkeycar import utils
from donkeycar.parts.kinematics import differential_steering
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be that you missed to check in that module - this seems to cause the break of 3 tests: test_actuator, test_template, test_web_socket.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. the import of differential_steering is not necessary; in the branch I am picking from it have been move to the kinematics parts, but not in this branch. I've removed the import.

donkeycar/parts/actuator.py Outdated Show resolved Hide resolved
@Ezward
Copy link
Contributor Author

Ezward commented Aug 6, 2022

@DocGarbanzo I don't know what is up with the tests; I don't understand the output from github. When I run the tests on raspberrypi I found an error in the actuator test. I fixed the actuator tests. test_template.py also passes on my raspberrypi. I don't think I broke the web_socket tests. They fail on main as well on my raspberrypi. It seems to be some async issue. I can see the value is set correctly in the web socket application, but at the time it is asserted in the test it is still the Mock. I've spent hours on this and I am stuck.

I realize I should have been looking at Mac or Ubuntu. On mac I see the errors as

====================================================================== short test summary info ======================================================================
ERROR tests/test_actuator.py - AttributeError: 'NoneType' object has no attribute 'BCM'
ERROR tests/test_pipeline.py - TypeError: Cannot create a consistent method resolution
ERROR tests/test_template.py - AttributeError: 'NoneType' object has no attribute 'BCM'
ERROR tests/test_torch.py - TypeError: Cannot create a consistent method resolution
ERROR tests/test_train.py - TypeError: Cannot create a consistent method resolution
ERROR tests/test_web_socket.py - AttributeError: 'NoneType' object has no attribute 'BCM'

The issue is the RPi.GPIO is not imported on Mac or Ubuntu, so the symbol GPIO.BCM does not exist. So I will go address that.

@@ -166,60 +180,6 @@ def set_pulse(self, pulse):
def run(self, pulse):
self.set_pulse(pulse)

class VESC:
Copy link
Contributor Author

@Ezward Ezward Aug 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased against main. For some reason it shows me deleting the VESC class. My local copy of actuator has VESC in it. It won't let me add it or commit it (Everything is up to date). What is up with that? Any idea? If this get's merged is it going to delete VESC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied it back in from main to fix this issue.

@Ezward Ezward requested a review from DocGarbanzo August 6, 2022 03:37
@Ezward
Copy link
Contributor Author

Ezward commented Aug 6, 2022

The mqtt test passes on my mac with python 3.9.

======================================================================== test session starts ========================================================================
platform darwin -- Python 3.9.10, pytest-7.1.1, pluggy-1.0.0
rootdir: /Volumes/APFS256/projects/donkeycar/donkeycar/tests, configfile: pytest.ini
plugins: cov-3.0.0
collected 1 item                                                                                                                                                    

tests/test_telemetry.py::test_mqtt_telemetry PASSED                                                                                                           [100%]

I reran the test and it gives this warning before running the test:

Mamba support is still experimental and can result in differently solved environments!

Copy link
Contributor

@DocGarbanzo DocGarbanzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I think we have to ignore the mamba warnings for now. We know it's not as safe as conda, but the download speed using conda has become so slow as you remember, so we don't have a choice atm.

@DocGarbanzo DocGarbanzo merged commit 3697313 into main Aug 9, 2022
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.

None yet

2 participants