-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Steering does not work when using drive , but does in calibration . Earlier commit works . #989
Comments
So that change got made to docs because we almost always tell folks to use dev when they have a problem. However, I believe you have found a pretty big bug in dev; we have duplicated this in another branch that branched from dev. As a work around, I think the I2C_SERVO drivetrain will work; it worked in that other branch. Thanks for reporting this bug. |
I'm going to revert that change to docs because it will lead folks to a bad experience. We will leave this bug open so we fix that actual issue in dev. |
We have a bug in dev branch that causes the default drivetrain to not operate correctly autorope/donkeycar#989 This commit reverts the change from December where we started specifying dev branch.
Thanks for the fast response @Ezward . This is a really cool project! I appreciate the work you all are doing improving it and keeping it going 🥇 |
SBC: Nano 4GB I am a DC Maintainer and have duplicated your issue using DRIVE_TRAIN_TYPE = "PWM_STEERING_THROTTLE" with both a gamepad (PS4) and the webui when running the 'path_follow.py' template. TCIII |
I believe what is going on here is that the PWM_STEERING_PIN configuration values is used in several DRIVETRAIN_TYPE configurations, each with a different default value. If you just set the DRIVETRAIN_TYPE = "PWM_STEERING_THROTTLE", but don't uncomment the PWM_STEERING_PIN value, then the default of a different drivetrain get's used. So a workaround is to just uncomment PWM_STEERING_PIN in the PWM_STEERING_THROTTLE sections. I will create a real fix and make a pull request for dev. |
Proposed fix is in this branch. https://github.com/autorope/donkeycar/tree/989-namespace-drivetrain-configurations @c1505 Corey if you get a change to try this it would be appreciated. |
@Ezward Thanks ! I'll give it a try today or tomorrow. Want to open a draft PR as well to have the automated tests kick off and to make the changes more easily visible ? |
Sorry I had a busy week and am away this weekend. I will give it a shot Monday. Thanks again for the fix! |
I tried:
with no other changes and it did not work. Then I...
That worked. Thank you. |
@BrianHenryIE can you give me a little more info on this; what did you do? Did you create a new mycar? Did you update your prior mycar using |
I did not create a new mycar, nor did I update. (my existing mycar was vanilla – I had reinstalled from scratch as I tried to troubleshoot). The fail was in the same way, i.e. no error message, just no working steering when launched. |
Ok, then it failed because there was no change to myconfig.py; so it failed in the same way. The issue is with how myconfig.py was being generated (with duplicate values in different sections). Thank you for the details. |
* Use python dict to namespace the various drivetrain configurations - There is one configuration for choosing the drivetrain. - However, each drivetrain has it's own lengthy set of configurations. - Some of these are shared among several drivetrains OR to avoid that they are prefixed and so identifiers become very long. - This commit moves each drivetrain's configuration into a python dict, effectively namespacing the configuration. - This allowed me to shorten the names of some configs since a prefix is not needed to disambiguate them. So some config names have changed - The bigger issue for users is that now, if they want to change any drivetrain config, they will need to uncomment the entire dictionary block, not just a single line of configuration. That should be obvious when looking at the code, but it could trick a newbie that does not understand python. I would say that is fine, because they will learn something important from it. - If you look at the resulting config.py you will see that it is much cleaner. * path_follow.py only support I2C_SERVO drivetrain - so I updated the configuration to reflect this - removed the DRIVETRAIN_TYPE configuration, since it is not used - removed the other drivetrain configurations which are not used * Change webui info log to debug log * Put drivetrain config in dictionary to address isssue #989 * remove spurious beta character and extra commas. * version="4.3.6" Co-authored-by: Ezward <Ezward@github.com>
Appreciate the fix @Ezward . I tested it with a new car for the steering and throttle for driving. If you don't uncomment everything in the PWM_STEERING_THROTTLE dictionary in the myconfig.py file, it fails with a key error, but otherwise works. I imagine that could be addressed in code, but I also think most people should be able to figure it out if they look at the error message. Unless there was a different fix that was put into the release, I think this fix should also be in the release. Otherwise, I the release is broken for at least nano users and based on the discord message I saw, possibly also raspberry pi folks. |
* Use python dict to namespace the various drivetrain configurations - There is one configuration for choosing the drivetrain. - However, each drivetrain has it's own lengthy set of configurations. - Some of these are shared among several drivetrains OR to avoid that they are prefixed and so identifiers become very long. - This commit moves each drivetrain's configuration into a python dict, effectively namespacing the configuration. - This allowed me to shorten the names of some configs since a prefix is not needed to disambiguate them. So some config names have changed - The bigger issue for users is that now, if they want to change any drivetrain config, they will need to uncomment the entire dictionary block, not just a single line of configuration. That should be obvious when looking at the code, but it could trick a newbie that does not understand python. I would say that is fine, because they will learn something important from it. - If you look at the resulting config.py you will see that it is much cleaner. * path_follow.py only support I2C_SERVO drivetrain - so I updated the configuration to reflect this - removed the DRIVETRAIN_TYPE configuration, since it is not used - removed the other drivetrain configurations which are not used * Change webui info log to debug log * Put drivetrain config in dictionary to address isssue #989 * remove spurious beta character and extra commas. * version="4.3.6" Co-authored-by: Ezward <Ezward@github.com> (cherry picked from commit cffec78)
Yes, need to update docs to instruct user to uncomment the entire dictionary block for the chosen drivetrain. What would be better is that they are just uncommented to start, since they do not conflict. Then setting the DRIVETRAIN_TYPE will just choose the right one. I'll look and see if we can do that. |
Issue description
Steering does not work when using drive , but does in calibration and it works when using an earlier commit. Seeing that another person in discord reported the same problem and that downgrading to 4.2.1 fixed it , I tried a more recent commit
d5b91301ff904873e70b9caba72823fe3454feb6
. It fixed the problem. I am using a Jetson nano 2gb and the other person has a raspberry pi so it seems to not be related to the particular board used. I went back and reinstalled dev and created a new car to make sure that the problem replicated for me before opening this issue.Steps to reproduce the issue
python manage.py drive
What's the expected result?
What's the actual result?
Additional details / screenshot
d5b91301ff904873e70b9caba72823fe3454feb6
is from December 30th of 2021. I have not tested other commits yet. I figured that someone more familiar with the code would be able to narrow down where the problem originated better than me.dev
. The rest say to install from master which seems like it is the same as the most recent release and I am assuming would have also worked for me.dev
ormaster
for the jetson nano and what should it be for the other devices? @Ezward it looks like you made that change . If you could provide feedback, that would be helpful.The text was updated successfully, but these errors were encountered: