945 refactor motor drivers #951
Conversation
|
Does not run on a NVIDIA Nano/Xavier as pins.py attempts to import RPi.GPIO as GPIO on a Nano/Xavier. |
That is a bug in dev branch as well. I've fixed it and pushed change to this branch. |
There was a problem hiding this comment.
@Ezward - I think this is a great change, only minor comments / questions. In general I observed, that you don't use f-strings or logging and your line lengths are not generally pep-8 compliant. Maybe you can change that?
donkeycar/parts/pins.py
Outdated
| mix pin schemes. | ||
| """ | ||
| prev_scheme = GPIO.getmode() or pin_scheme | ||
| GPIO.setmode(pin_scheme) |
There was a problem hiding this comment.
Just an observation here - our PWM actuator parts are multithreaded for performance reasons so they don't clog up the vehicle loop. I hope there is no issue if these parts will talk to the actuators from different threads.
There was a problem hiding this comment.
Yes, I don't like this pattern where GPIO is a singleton with shared state. I agree this could be an issue if there are pins using different schemes. One approach would be to require a global scheme to be set then just check against it and runtime fail in pin spec does not match. Another approach is to use a semaphore/lock to make sure this section is not accessed by more than one thread.
donkeycar/parts/pins.py
Outdated
|
|
||
| def start(self, state:int=PinState.LOW) -> None: | ||
| if self.state() != PinState.NOT_STARTED: | ||
| raise RuntimeError("Attempt to start OutputPin that is already started.") |
There was a problem hiding this comment.
Maybe add the PIN number to the exception message, so we know which ones is causing a problem.
There was a problem hiding this comment.
...? not in this version.. maybe still on your local change...
donkeycar/utilities/deprecated.py
Outdated
| return new_func2 | ||
|
|
||
| else: | ||
| raise TypeError(repr(type(reason))) |
donkeycar/parts/pins.py
Outdated
| try: | ||
| self.pwm.set_pwm(channel, 0, pulse) | ||
| except: | ||
| self.pwm.set_pwm(channel, 0, pulse) |
There was a problem hiding this comment.
This looks spurious, if the try failed we should log an error, but not call again w/o try/except.
There was a problem hiding this comment.
I ported this (it was already in our code). Looks like a one-time retry. I'll check with the original author why it is there, but I don't want to remove it without understanding that. I can't really hurt leaving it, but it might hurt taking it out.
There was a problem hiding this comment.
I think it was Doug in d1efdfa, and the code was in the PCA9685 before. I have taken this bit out of my fork a long time ago, as I saw regular issues where it was throwing from the except branch, because the I2C can sometimes produce a hick up. If a single pwm signal gets missed, we are just a missing a control for 1/vehicle_HZ seconds, so not too bad.
There was a problem hiding this comment.
@DocGarbanzo Is there a reason not to do this retry?
There was a problem hiding this comment.
Yes, if there is an I2C error, which is something I have observed on my car, then the non-exception handled retry might throw which will stop the vehicle loop. The except code should simply log an error, and when the vehicle loop comes around again, it will try to set it again.
| pulse = int(4096 * duty_cycle) | ||
| try: | ||
| self.pwm.set_pwm(self.channel, 0, pulse) | ||
| except: |
There was a problem hiding this comment.
This issue is still here....
There was a problem hiding this comment.
I"m ok with the except: clause doing a retry.
donkeycar/parts/pins.py
Outdated
| self.pwm.set_pwm(channel, 0, pulse) | ||
| except: | ||
| self.pwm.set_pwm(channel, 0, pulse) | ||
| self.pwm.set_pwm(channel, 0, pulse) |
There was a problem hiding this comment.
I think this we should keep the try/catch and incorporate proper exception handling like:
try:
self.pwm.set_pwm(channel, 0, pulse)
except Exception as e:
logger.error(f'Error in talking to PCA9685 channel {channel}: {str(e)}')- abstract classes InputPin, OutputPin and PwmPin define interface for pin types. - input_pin_by_id(), output_pin_by_id(), pwm_pin_by_id() are factories for creating pins using various underlying technologies. - the point is to decouple the control of things like motors or encoders from a particular pin library, so we don't have to create a whole new redundant part if we want to use a different set of hardward. - For instance, our L298N motor driver parts use RPi.GPIO for enable pins and pwm pins. This means if you have a robot that uses a PCA9685 to control the enable and pwm for your L298N controller, you have to write a whole new motor driver to use it. - This change makes it so we can make our parts that use ttl and pwm pins more reusable.
- this motor driver can use Rpi.GPIO or PCA9685 pins for enable and/or pwm pins, so we can use one driver for different pin technologies. - This will even allow a mix of underlying pin providers for a single motor. - For instance, the DuckieBot robots use Jetson Nano gpio pins for enable, but they use PCA9685 to generate pwm signals. With this new motor driver we can make Donkeycar work on that robot.
- donkeycar.parts.pins has high level pin api
- also made some fixes that I found when testing - currently it appears InputPinGpio and OutputPinGpio function correctly, but PwmPinGpio does not
- needed to call pwm.start(duty) to start duty cycle
- older code used 1 to 100 duty cycle value and so did some truncation to integer. That left over code cause throttles to be truncated to -1 or 0.
- added pin-provider configuration for mini-hbridge - lots of commenting so folks can understand the different motor drivers, how to wire them and how to configure them.
- Since DC_STEER_THROTTLE used the mini-hbridge driver it can use the new versino that uses pin providers. - update the config in cfg_complete.py as well
- This then makes the DC_STEER_THROTTLE, DC_TWO_WHEEL, DC_TWO_WHEEL_L298N drivers support PCA9685, Rpi.GPIO and pigpio pin providers
- added @deprecated decorator to mark classes and functions that are to be removed in the future - Added PulseController that uses pin provider api to provide the set_pulse() api needed by some steering/throttle controllers. - Marked some other set_pulse() controllers that are now redundant as @deprecated - added a lot of commenting on those classes that are deprecated, including why and how we may move ahead if we wish to keep the functionality. Some of these fall into code that is not actually integrated into a template and is not documented either in docs.donkeycar.com or the code. Some of this code, like the Arduino Firmata, could be usefully integrated into the pins.py pin provider api but others would be redundant or otherwise unnecessary.
update install dependencies for nano
nano was installed RPi.GPIO, but needs Jetson.GPIO.
'Jetson.GPIO',
'pyserial',
- 'speed' not defined; should be 'throttle' - use actuator.L298N_HBridge_2pin for both motors
- SERVO_HBRIDGE_2PIN supports steering servo and an HBridge motor driver using 2 pwm pins for motor controller - SERVO_HBRIDGE_3PIN supports steering servo and an HBridge motor driver with 2 ttl pins for direction and 1 pwm for duty cycle to control the motor.
- NOTE: I'm using a 120 character line
Fixed lint (Flake8) issues, update class/method docs - using 120 character line length
- prior code would try to change pin scheme per pin, however RPi.GPIO will only let pin scheme to be set once. - code now makes sure that pin scheme is set and does not change.
- the except clause did not try to differentiate the exception so the retry logic might not be the right thing to do there. - Now any exception will leak and we will actually see it so we can write appropriate code in the future if we see that happening.
c5ef9d6 to
fba7766
Compare
|
I've rebased against dev and updated the RC hat docs to reference the new drive train type. I've completed most of the tests in #945 (comment) . Those that are not completed are because of lack of hardware. However, each of those have minimal risk because we have tested aspects of those configurations when we tested other configurations. @DocGarbanzo I think this is good to go if you do. |
|
I hope you have re-based against |
|
@DocGarbanzo Yes, I rebased against dev (NOT master, sorry. I've fixed the comment). I've bumped version to version="4.3.3". Thanks for all of your input on this complex branch. |
Addresses Issue #945
Companion documentation PR
This should be QA'd in conjunction with the documentation PR using the test plan in Issue 945
NOTE: I expect and desire a lot of input on this pull request. If you like it and want to approve it, then great. But please don't merge it. I'd like to get consensus on this prior to merge.
This is a large refactor of the motor drivers. The goal is to make the motor drivers more reusable. I got the itch to do this because I have a Duckiebot differential drive robot. In another branch I'm adding odometry/kinematics and pose estimation, but I could not test it on this bot; Duckiebot uses an L298N 3pin kind of motor setup and we have a drive configuration, DC_TWO_WHEEL_L298N, that almost works. The issue is that this bot uses a PCA9685 to generate the duty cycle. Actually it's worse than that, it mixes some TTL outputs from the GPIO and the PCA9685. So our driver would not work, since it assumes only GPIO output (using RPi.GPIO library) for both TTL and PWM. So I was in the position of writing yet another one-off driver. I realized that if the differential drive code took a higher-level pin specification, then it would not be tied to any particular hardware or library. Looking at actuator.py, there are a number of instances where we have almost the same driver logic, but re-implemented for different libraries or pwm hardware. Worse, they diverge a little in how they actually work. This then causes a proliferation of configuration options in myconfig.py. There is very little documentation how the hardware maps to a particular drivetrain so how does a user know which drive train to choose for their hardware? There are a number of classes for particular microcontrollers or pwm generators that are not actually hooked up or documented; they are just latent in the code; no one would know they are there or how to use them looking at the templates or myconfig.py.
So I did not want to add to this problem. What I tried to accomplish was to refactor so we only need a few reusable drivetrains; one for servo/esc, One for a server plus L298N, one for differential drive L298N 3pin wiring and one for differential L298N 2pin wiring. These drivers take pin-specifiers rather than raw pin numbers. Pin specifiers indicate the source for the pin; PCA9685, RPi.GPIO/Jetson.GPIO or PIGPIO and other particulars. These can all be used on raspberry pi or jetson nano, with our without a PCA9685. This allows us to drastically cut down on the code in actuators.py and complete.py and the amount of configuration in config.py.
So this is pretty opinionated pull request. Actuator.py has a lot of cruft in it. I would like to see it paired down to what we really intent to support in the framework. For those other edge cases, like a particular microcontroller implementation, I think those should be in their own repository with a readme that documents what they are and how the donkey code would need to be changes to use them. Maybe that repo in is in autorope, maybe it should be in the author's own github account if it is particular to them. If there is another way to handles these things, please let me know.
This pull request is the first of several I have lined up, so I hope to get a lot of input on this. That will help guide me in how I manage the subsequent pull requests. I would enjoy a lot of input. I this this also needs a lot of testing, so I have when you are reviewing this that you will pull the code and try it on your donkeycar.
Here is the approach I took:
create a layer of abstraction over input/output/pwm pins. I call this the pin provider api and it can be found in donkeycar/parts/pins.py. There are 3 pin providers currently implemented; PCA9685, Rpi.GPIO (and so Jetsion.GPIO) and PIGPIO. I'm trying to figure out of we need to support ServoBlaster and Arduino Firmata (or possibly our own Arduino sketch). Pins are specified using a pin-spec string. For instance, "PCA9685.1:40.13" specifies channel 13 on the PCA9685 at address 0x40 on I2C bus 1. "RPI_GPIO.BOARD.33" specified board pin 33 driven by the RPi.GPIO library. "PIGPIO.BCM.20" specifies broadcom gpio-20 driven by the pigpio library.
Refactor pwm controllers (those support set_pulse() methods) to use this new system, so we can eliminate the other library specific implementations that are tied to specific configurations. This allows us to be more general in our support of these underlying hardware architectures without users having to write their own parts. For instance, the
PIGPIO_PWMconfiguration supports an hardware layer that takes pwm pulses for both steering and throttle as you would do for an RC car with a servo and esc. However, it requires the use of the PIGPIO libary, which does not work on Jetson Nano. There is now a drivetrain named PWM_STEERING_THROTTLE and a generalized pulse controller called PulseController that uses the pins.py pin provider api to send servo style PWM pulses to Servo/ESC style RC car drivetrains. This means we can retire PIGPIO_PWM and it's code (and I2C_SERVO drive train configuration). A car with a servo/esc drivetrain can now be driven by RPi.GPIO, PIGPIO or a PCA9685 using just the PWM_STEERING_THROTTLE drivetrain configuration.Refactor non-pwm-controller configurations, like the
DC_TWO_WHEELandDC_TWO_WHEEL_L298N, to use pin providers. This has those same benefits; now a differential drive robot can use the GPIO header or a PCA9685 for PWM. In fact, it can mix the two; I have a Duckiebot that does just that; now it can run donkeycar. Duckies and Donkeys living together in peace!Delete unused code. We have a bunch of code that is not hooked up to anything OR is supporting boards that we really don't support. So we have a bunch of code that is not in a template and has no configuration. The only way to use it is to edit your manage.py (or create a fork and edit complete.py). So that makes actuators a dumping ground for individuals' pet projects that are not really part of the framework. That makes actuators much harder to maintain. For the most part, I have not actually deleted this, but I have marked it as @deprecated and added a comment that describes why I think it should be deprecated and how we might actually rescue this code and make it part of the framework. So with the exception of the DC_TWO_WHEEL** drivetrains, the old drivetrain code remains in place, but is marked as deprecated. This allows us to merge this code into the 4.x branch without breaking changes (except to the DC_TWO_WHEEL drivetrains, as mentioned. If we think these should also not be touched, I can put the original code back in and create a parallel set of configurations for the new pin-provider versions). Come version 5.x, I believe we should either delete the deprecated code or make an effort to make it part of the framework.
Add a lot more commenting and documentation while we are at it. I've added a lot of comments to the code. There will be a second pull request for donkeydocs after this pull request has gone through some review. Again, given that the old configurations are generally maintained, the old docs are still valid.
Here is a high level explanation of the file changes:
donkey calibratecommand for pin provider. I've tested this with a PCA9685 servo/esc setup. I could use help testing other setups.Verification before merging: