photodude / DualVNH5019MotorShieldMod3

Arduino library for the Pololu Dual VNH5019 Motor Driver Shield
http://www.pololu.com/product/2507
Other
5 stars 5 forks source link

Motor 4 functions should use OCR5A, not OCR5B #6

Closed samascaro closed 5 years ago

samascaro commented 5 years ago

The setM4Speed() and setM4Brake functions are currently trying to use OCR5B, which is the same one motor 3 is trying to use. If motor 4 is on Pin 46, then the motor 4 functions should be using OCR5A.

photodude commented 5 years ago

@samascaro you are right, This line should be OCR5A not OCR5B

I've been debating rewriting the PWM in this library to utilize the Timer libraries in Arduino (TimerOne, TimerTwo, TimerThree, TimerFour, and TimerFive search the Arduino library manager) The Timer libraries give PWM timer frequency controls beyond what is integrated into this library. I haven't personally dove into working with them and this library; but those libraries will likely be the best option for controlling all the other PWM frequency timers for the other pins (likely need to comment out all the Timer stuff in the .cpp when using the Timer libraries). I do recommend reading over the Notes about timers and library conflicts: as it covers many use cases where this library and things like the servo libraries can conflict creating problems.

I'll make the change and push an update soon. Go Utes.

photodude commented 5 years ago

@samascaro Version 4.0.0 has been released with this fix. Note there was a Backward Compatability break between Version 3.0.2 and version 4.0.0 to fix the PWM pins. If you were working from the master branch you already had this fix. details on the change are in the release notes and this commit 227a49b9fa22a2383e78f0dc959e80dfd2bb7d6c

Closed via fe7176483a4174ba94d963023a13fe5075bf79af

samascaro commented 5 years ago

Thanks for this library. My Mechatronics students are all using two of these Pololu Motor Drivers on their Arduino Megas. However I'm not crazy about switching PWM1 and PWM2 to pins 11 and 12 in the new release (used to be 9 and 10). This requires my students to rewire the Motor Shield for Motors 1 and 2, which is a pain. Is there any reason not to stick with pins 9 and 10 and use Timer2 instead of Timer1?

photodude commented 5 years ago

@samascaro I understand. Mechatronics students at the UofU have been using this library with two of these Pololu Motor Drivers on their Arduino Megas since I wrote this modified version in the old Mechatronics sequence.

If I remember right, the change to the default pins was motivated by Timer2 being 8 bit and Timer 1 and the other Timers being 16 bit. That being said I think I forgot that I was also going to change pins 45 and 46 to something else as modifying the PWM on Timer 5 causes library conflicts with the servo library on the mega. I believe we worked around that by using software sevo. I might move the 4.x release to a pre-release status until I complete the other pin changes.

I've created a new branch 3.x-old-pins and I'll work through getting a release ready this afternoon.

photodude commented 5 years ago

@samascaro I have pushed version 3.1.0 from the new branch 3.x-old-pins branch.

Feel free to let me know about any other bugs or feature requests. I also take Pull requests. As mentioned above in regards to library conflicts with various timers I've outlined the details I have discovered since writing this library and running into conflicts with the servo library.

Another note about this library. I wrote this at a time when I hadn't taken Programming for Engineers and had not been exposed to the Object-Oriented Programming (OOP) design pattern. The goal of this library was to simplify my access to controlling the motors without dealing with multiple library instances.

To use the Pololu library at OOP to control the motors you instantiate two copies of the Pololu library.

// OOP Initialize the Pololu library for each dual VNH5019 motor driver
DualVNH5019MotorShield MotorsAB(INA1, INB1, PWM1, EN1DIAG1, CS1, INA2, INB2, PWM2, EN2DIAG2, CS2);
DualVNH5019MotorShield MotorsCD(INA3, INB3, PWM3, EN1DIAG3, CS3, INA4, INB4, PWM4, EN2DIAG4, CS4);

The disadvantage to the OOP instantiation method is you have to write your own functions to control all four motors at the same time (speed, brake, etc) as each instance of the library will only control 2 motors. That is why this library is still helpful, you have one library and access to all four motors without needing to write any additional functions. This is also why I still maintain the library, it has a specific use case which is very useful to many people.

samascaro commented 5 years ago

Thanks. I also thought of the option to instantiate the original Pololu libary twice, but I think the students will appreciate having a single library for all four motors. I approve of your version 3.1.0.

As far as using pins 9 and 10 for PWM, it seems that we are doing fine with defaulting to analogWrite on those pins. However I'm curious if it would make a big difference to use Timer 2 (8-bit) vs. Timer 1 (16-bit). Since your top OCR value is 400, you're not really getting much more than 8 bit resolution on your PWM. Unless I'm missing something, using Timer 2 would have same resolution as analogWrite, but potentially faster frequency. For example, with a prescaler of 8, you'd get f=16MHz/8/2/256 = 3.9kHz. Would that be fast enough to get reliable current measurements? Maybe something to investigate for a future release....

As far as using pins 45 and 46, it seems we can avoid the conflicts with the Servo.h library by using the PWMServo.h library instead. Maybe it is best to use PWMServo.h anyway since that one advertises it is immune to interference from other libraries that use interrupts. (We will be using the Encoder.h library, which makes use of interrupts).

photodude commented 5 years ago

@samascaro I agree, students would likely prefer the single library and not needing to write additional functions to get all of the functions in this single library. Easier to hunt bugs in their own code.

Pins 9 and 10 defaulting to analogWrite() might have been part of why I planned to move the default pins. It seemed like a simple fix to actually get PWM on shield 1. The existing PWM frequency calculation and default pins 9, 10 for shield 1 are carryovers from the Parent Pololu library which was set up for the Uno and those pins on the Uno are on a 16bit Timer. The other default pins for shield 2 were also arbitrary choices when I wrote this.

I agree, the PWMServo.h library would likely save a lot of headaches.

samascaro commented 5 years ago

Here's a version I made that uses Timer2 with pins 9 and 10 on the Mega for controlling motors 1&2. The resulting PWM frequency is 7.8 kHz, which eliminates the annoying audible whine that you get from the motors when defaulting to 490 Hz PWM via analogWrite.

DualVNH5019MotorShieldMod3.zip

photodude commented 5 years ago

@samascaro Awesome, I'll take a look at it when I can, but it probably won't be until after design day. Feel free to submit a pull request to the 3.x branch with the changes and I'll review and merge it as soon as possible.

photodude commented 5 years ago

@samascaro I found some time today and I have made the changes to use timer 2 at 7.8 kHz for the 3.2.0 version of the library. Please try version 3.2.0 to see if that meets your student's needs. Good luck with your design day competition.