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

Motor Driver - Code Style and Timers #37

Closed
McNugget6750 opened this issue Nov 24, 2019 · 55 comments
Closed

Motor Driver - Code Style and Timers #37

McNugget6750 opened this issue Nov 24, 2019 · 55 comments

Comments

@McNugget6750
Copy link

Hi there,

I love how much work has been put into this all to make this work and open source.
The whole day, I have been reading the Arduino code for the motor driver and added a ton of documentation, changed the layout a little bit and organized the code based on configuration and pin assignments as well as important stuff at the top and tools etc. at the bottom of the file.

However, I wonder, why you are using all the AVR native functions and not the Arduino build in libraries. Is it really necessary to drive this at 60Hz? Is it really necessary to run certain things at exactly 30Hz? Or 50Hz?
Why did you decide to configure the code by using external jumpers if the code needs to be modified to fit ones needs anyways?

I guess I would like to propose to make this a bit more modular. I want to implement compatibility with the IBT 2 H-BRIDGE. It's cheap and powerful but it needs two PWM and two enable lines but it also provides two current sense / fault lines. Integrating this into your code seems incredibly complex as it requires detailed knowledge of the AVR timers. It looks like I can't just use analogWrite() to do this because you're running custom timers.

If this code could be restructured to use #defines in a config.h to do the general configuration, a pins.h to do all pin assignments and remove all the #if 1 or #if 0 ... #endif stuff, and removal of all the custom timers, it would already help a lot. At present, without further documentation, I would love to rewrite all this to use standard Arduino functions instead of lowest level AVR code. I mean, who really uses a bare metal H-Bridge these days that would require all this in the first place?

Please don't take this the wrong way! I know people are using and loving your work! And I'm here for a reason: I want to use OpenPlotter and PyPilot, too! However, in order to make this motor driver more modifiable, we need to clean up the code and write more documentation :)

@seandepagnier
Copy link
Member

I'm not sure what you mean by 30hz or 50hz, but a reasonable reaction speed is very useful. The arduino can react much faster than the pi to certain faults (like reflex pulling away from being burned)

I use the external jumpers. I have 4 different versions of actual hardware I have built so the software never needs to be modified and there is not a problem of flashing the wrong version and needing to tweak the config.

Unfortunately there are only so many io available for these straps, so if you use a different module you might need to recompile.

I am using custom timers, yes. This is needed. The arduino is clocked down to 4mhz, but the pwm frequency needed is 16khz. So to properly switch the mosfets and also control the deadtime it is needed to use interrupts actually making them naked to save cycles and give a better duty cycle.

I agree that you could simplify the code perhaps somewhat but end up with an inferior driver on the same hardware.

I am happy to accept a pull request that improves the code.

@McNugget6750
Copy link
Author

Thank you very much for the quick response!
Maybe you can give me a 2 minute intro to what is the rudder control code actually doing and I will figure out the timers.

Since your version is considered to be currently usable and stable, I will make a branch or fork and make changes there. The main difference is that your code is optimized to work with bare metal H-Bridges and my code does not need that functionality. That's why I want to separate the versions and create one that uses generally the same functions and interfaces (especially the uart communication) but rewrite everything that requires custom timers for the following reason:

The already supported vnh2sp30 doesn't need it and my proposed IBT2 H-Bridge also doesn't need it. These can simply be operated using one or two PWM outputs (analogWrite()) and a bunch of IOs (digitalWrite()).
My assumption here is that almost everyone new to the project will use an off the shelve h-bridge from Amazon or Ebay instead of using a (potentially custom build) bare metal H-Bridge that requires the exact timing constraints you have developed.

Questions:

  1. The rudder sensor is read and the max travel is taken into account. However, I coulnd't find an actual rudder controller, yet. It looks like the rudder sensor gives absolute values while the command coming from PyPilot is a desired PWM with the center being 1000? Like an RC servo.

  2. The engage() function always first resets position(1000); instead of ramping from one value to the next. Wouldn't that cause a lot of jerk? Since every single time a new COMMAND_CODE is processed in void process_packet() the function engage() is called.

My understanding:
void engage() enables all required PWM channels for the different motor drivers, engages the clutch and turns on the LED. PWM is reset to 1000 by default.

void disengage() stops the rudder, turns the engage flag off and turns off the above LEDs

void detach() turns off all PWM generators

void update_command() takes the last value (lastpos) and drives it towards the commanded value (some value between 0 and 2000), applies a slew rate to it and sends it to position()

Now the interesting part that I don't yet understand
void position() takes in value and (in case of pwm_style == 2) drives the enable OI on the vnh2sp30 either CCW or CW and that's it!
What is "controlling" on the rudder position? Is that PyPilot? Because it looks that way.
My understanding is that the rudder position is reported back to PyPilot and PyPilot then sends out a desired PWM value between 0 and 2000 to either just drive the motor CCW or CW. PyPilot maintains complete control over the PID (or similar) controller and motor.ino only really drives PWM and reports telemetry. Is that understanding correct?

Regarding your questions:
In loop() at the beginning you run through a series of if-statements to ensure update_command() is called at 30hz. First you check for 50hz and then you count another couple cycles to then call update.

At the end of loop() you send the telemetry back to PyPilot at ... it seems ... as fast as you can. Is that correct? So it basically sends data all the time with no pause in between packages?

How often does PyPilot send a new data package to motor.ino?

@seandepagnier
Copy link
Member

seandepagnier commented Nov 24, 2019

The 50hz loop controls the slew rates so that they work correctly.

The telemetry is sent as fast as commands are received, so pypilot controls the data rate. This gives a higher data rate when pypilot is run at 25hz instead of the default 10. pypilot usually sends 2-3 packets (4 bytes each) per iteration.

rudder feedback is optional and pypilot works without. The motor controller does prevent further movement outside rudder limits which gives a slightly quicker reaction than using python for this. It basically works ok to use python for this control which is the case if the rudder feedback is supplied via nmea to pypilot rather than through the controller.

The engage function does not jerk or reset speed because at first it does nothing if already engaged:

if(flags & ENGAGED) {
//update_command(); // 30hz
return;
}

@McNugget6750
Copy link
Author

Thanks for the additional explanation! I'm digging through this right now.

@McNugget6750
Copy link
Author

McNugget6750 commented Nov 25, 2019

Could you please help me understand what you are doing with rudder_sense? I'm specifically interested in understanding the bottom portion of the code

`const int rudder_react_count = 40; // approx 0.1 second reaction
if(CountADC(RUDDER, 1) > rudder_react_count) {
uint16_t v = TakeRudder(1);
if(rudder_sense) {
// if not positive, then rudder feedback has negative gain (starboardersed)
uint8_t pos = rudder_min < rudder_max;

        if((pos && v < rudder_min) || (!pos && v > rudder_min)) {
            stop_starboard();
            flags |= MIN_RUDDER_FAULT;
        } else
            flags &= ~MIN_RUDDER_FAULT;
        if((pos && v > rudder_max) || (!pos && v < rudder_max)) {
            stop_port();
            flags |= MAX_RUDDER_FAULT;
        } else
            flags &= ~MAX_RUDDER_FAULT;

        if(v < 1024 || v > 65472 - 1024) // MAGIC NUMBERS?
            rudder_sense = 0; // What is this used for?
    } else {
        if(v > 1536 && v < 65472 - 1536) // MAGIC NUMBERS?
            rudder_sense = 1; // What is this used for?
        flags &= ~(MIN_RUDDER_FAULT | MAX_RUDDER_FAULT);
    }
}`

@McNugget6750
Copy link
Author

I generally got the system working. It communicates and I see my rudder sensor when I move the rudder. It engages but I get a DRIVER_TIMEOUT displayed in Autopilot Control. When is this set and why?
I have not tested any h-bridge control, yet/

@seandepagnier
Copy link
Member

This is a rather confusing flag which indicates that no current (amps) is detected and therefore the motor is likely disconnected. This would obviously be the case if no hbridge is attached.

maybe it should be renamed to NO_MOTOR ?

@CapnKernel
Copy link

CapnKernel commented Nov 25, 2019

@McNugget6750 writes:

reading the Arduino code for the motor driver and added a ton of documentation, changed the layout a little bit and organized the code based on configuration and pin assignments as well as important stuff at the top and tools etc. at the bottom of the file.

I guess I would like to propose to make this a bit more modular. I want to implement compatibility with the IBT 2 H-BRIDGE. It's cheap and powerful but it needs two PWM and two enable lines but it also provides two current sense / fault lines.

Hi Timo. We should talk. I'm also doing this right now (I want to integrate IBT-2 support), and I'm concerned that we're going to end up doing duplicate work which can't be reconciled.

I have mostly finished changes to make the choice of driver be a compile-time thing, in a way that we can add support for new drivers (you and I want IBT-2, and @partyvi is working on TI DRV8873H).

I believe/hope we can get away with only having one current sense line (I talk about it here):

http://forum.openmarine.net/showthread.php?tid=1840&pid=10976#pid10976

And I think the two enable lines can be joined and driven as one.

@seandepagnier writes:

I use the external jumpers. I have 4 different versions of actual hardware I have built so the software never needs to be modified and there is not a problem of flashing the wrong version and needing to tweak the config.

Unfortunately there are only so many io available for these straps, so if you use a different module you might need to recompile.

If we want to continue to be able to select this at run time, one way might be to use certain values of resistor on an analog pin. motor.ino can read the value (voltage divider) and work out what hardware to support. Alternatively, we could add a command to the protocol to set the driver, and this gets persisted in EEPROM. PyPilot could offer a UI option to allow setting it.

Anyway, here is my unfinished work in progress. I don't have hardware to test it on yet, but I'm running it through the preprocessor for each driver, and comparing the output to the original.

diff --git a/arduino/motor/motor.ino b/arduino/motor/motor.ino
index e586e74..8829f44 100755
--- a/arduino/motor/motor.ino
+++ b/arduino/motor/motor.ino
@@ -16,10 +16,13 @@
 #include <util/delay.h>
 
 /*
-This program is meant to interface with pwm based
-motor controller either brushless or brushed, or a regular RC servo
+This program talks to a motor controller.  Several kinds are supported.
+ - pwm based, either brushless or brushed
+ - regular RC servo
+ - vnh2sp30
+ - others coming
 
-You may need to modify the source code to support different hardware
+You can modify the source code to support new controllers.
 
 adc pin0 is a resistor divider to measure voltage
              allowing up to 20 volts (10k and 560 ohm, 1.1 volt reference)
@@ -39,14 +42,12 @@ D4  D5
  1   0        .0005 ohm x 50 gain
  0   0        .0005 ohm x 200 gain   *ratiometric mode
 
+For the H bridge, digital pins 2 and 3 are for the low side, 
+pins 9 and 10 are for the high side.
 
-digital pin6 determines:
-1 - RC pwm:
-   digital pin9 pwm output standard ESC (1-2 ms pulse every 20 ms)
-           pin2 esc programming input/output (with arduinousblinker script)
-0 - Hbridge
-   digital pin2 and pin3 for low side, pin9 and pin10 for high side
-
+For RC servo PWM, digital pin 9 is standard 1-2 ms pulse every 20 ms
+PWM output for ESC, pin 2 is for ESC programming input/output
+(with arduinousblinker script)
 
 optional:digital pin7 forward fault for optional switch to stop forward travel
 digital pin8 reverse fault for optional switch to stop reverse travel
@@ -114,11 +115,35 @@ PWR+             VIN
 */
 
 
+#define DRIVER_NOT_SET 0
+#define DRIVER_HBRIDGE 1
+#define DRIVER_RC_PWM 2
+#define DRIVER_VNH2SP30 3
+
+// #define DRIVER DRIVER_NOT_SET
+// REmove
+#define DRIVER DRIVER_HBRIDGE
+
+#if !DRIVER
+#  error You need to uncomment one of the above driver options.
+#endif
+
+#undef DISABLE_TEMP_SENSE    // has temp sensors
+#undef DISABLE_VOLTAGE_SENSE // has voltage sense
+#undef DISABLE_RUDDER_SENSE  // has rudder sense
+
+#if DRIVER==DRIVER_HBRIDGE
+#endif
+
+#if DRIVER==DRIVER_RC_PWM
+#endif
+
+#if DRIVER==DRIVER_VNH2SP30
+#  define DISABLE_TEMP_SENSE    // if no temp sensors avoid errors
+#  define DISABLE_VOLTAGE_SENSE // if no voltage sense
+#  define DISABLE_RUDDER_SENSE  // if no rudder sense
+#endif
 
-//#define VNH2SP30 // defined if this board is used
-//#define DISABLE_TEMP_SENSE    // if no temp sensors avoid errors
-//#define DISABLE_VOLTAGE_SENSE // if no voltage sense
-//#define DISABLE_RUDDER_SENSE  // if no rudder sense
 
 
 // run at 4mhz instead of 16mhz to save power,
@@ -170,10 +195,6 @@ uint8_t low_current = 1;
 
 #define ratiometric_mode (!shunt_resistance && !low_current)
 
-#define pwm_style_pin 6
-// pwm style, 0 = hbridge, 1 = rc pwm, 2 = vnh2sp30
-uint8_t pwm_style = 2; // detected to 0 or 1 unless detection disabled, default 2
-
 #define port_fault_pin 7 // use pin 7 for optional fault
 #define starboard_fault_pin 8 // use pin 7 for optional fault
 // if switches pull this pin low, the motor is disengaged
@@ -185,15 +206,19 @@ uint8_t pwm_style = 2; // detected to 0 or 1 unless detection disabled, default
 #define hbridge_b_bottom_pin 3
 #define hbridge_a_top_pin 9
 #define hbridge_b_top_pin 10
-#define enable_pin 10 // for vnh2sp30
+#if DRIVER==DRIVER_VNH2SP30
+#  define enable_pin 10
+#else
+#  undef enable_pin
+#endif
 
 // for direct mosfet mode, define how to turn on/off mosfets
 // do not use digitalWrite!
-#define a_top_on  PORTB |= _BV(PB1)
+#define a_top_on PORTB |= _BV(PB1)
 #define a_top_off PORTB &= ~_BV(PB1)
 #define a_bottom_on PORTD |= _BV(PD2)
 #define a_bottom_off PORTD &= ~_BV(PD2)
-#define b_top_on  PORTB |= _BV(PB2)
+#define b_top_on PORTB |= _BV(PB2)
 #define b_top_off PORTB &= ~_BV(PB2)
 #define b_bottom_on PORTD |= _BV(PD3)
 #define b_bottom_off PORTD &= ~_BV(PD3)
@@ -348,7 +373,6 @@ void setup()
 
     pinMode(shunt_sense_pin, INPUT_PULLUP);
     pinMode(low_current_pin, INPUT_PULLUP);
-    pinMode(pwm_style_pin, INPUT_PULLUP);
     pinMode(clutch_pin, INPUT_PULLUP);
     pinMode(voltage_sense_pin, INPUT_PULLUP);
 
@@ -376,9 +400,6 @@ void setup()
     pinMode(starboard_fault_pin, INPUT);
     digitalWrite(starboard_fault_pin, HIGH); /* enable internal pullups */
 
-    pinMode(pwm_style_pin, INPUT);
-    digitalWrite(pwm_style_pin, HIGH); /* enable internal pullups */
-
     pinMode(shunt_sense_pin, INPUT);
     digitalWrite(shunt_sense_pin, HIGH); /* enable internal pullups */
 
@@ -387,14 +408,6 @@ void setup()
 
     _delay_us(100); // time to settle
 
-    // test output type, pwm or h-bridge
-#ifndef VNH2SP30
-    pwm_style = digitalRead(pwm_style_pin);
-#endif
-    if(pwm_style) {
-        digitalWrite(pwm_output_pin, LOW); /* enable internal pullups */
-        pinMode(pwm_output_pin, OUTPUT);
-    }
     // test shunt type, if pin wired to ground, we have 0.01 ohm, otherwise 0.05 ohm
     shunt_resistance = digitalRead(shunt_sense_pin);
     //shunt_resistance = 0;  // for test board which doesn't have pin grounded
@@ -402,13 +415,16 @@ void setup()
     // test current
     low_current = digitalRead(low_current_pin);
 
-#if 1
     // setup adc
     DIDR0 = 0x3f; // disable all digital io on analog pins
-    if(pwm_style == 2 || ratiometric_mode)
+#if DRIVER==DRIVER_VNH2SP30
+        adcref = _BV(REFS0); // 5v
+#else
+    if(ratiometric_mode)
         adcref = _BV(REFS0); // 5v
     else
         adcref = _BV(REFS0)| _BV(REFS1); // 1.1v
+#endif
     ADMUX = adcref | _BV(MUX0);
     ADCSRA = _BV(ADEN) | _BV(ADIE); // enable adc with interrupts
 #if DIV_CLOCK==4
@@ -418,7 +434,6 @@ void setup()
     ADCSRA |= _BV(ADPS0) |  _BV(ADPS1) | _BV(ADPS2); // divide clock by 128
 #endif
     ADCSRA |= _BV(ADSC); // start conversion
-#endif    
 }
 
 uint8_t in_bytes[3];
@@ -432,14 +447,20 @@ uint8_t rudder_sense = 0;
 
 // command is from 0 to 2000 with 1000 being neutral
 uint16_t lastpos = 1000;
+
+#if DRIVER==DRIVER_RC_PWM
 void position(uint16_t value)
 {
     lastpos = value;
-    if(pwm_style == 1)
 //        OCR1A = 1200/DIV_CLOCK + value * 6 / 5 / DIV_CLOCK;
         OCR1A = 1500/DIV_CLOCK + value * 3 / 2 / DIV_CLOCK;
     //OCR1A = 1350/DIV_CLOCK + value * 27 / 20 / DIV_CLOCK;
-    else if(pwm_style == 2) {
+}
+#elif DRIVER==DRIVER_VNH2SP30
+void position(uint16_t value)
+{
+    lastpos = value;
+    // FIXME: Should deindent code in these three position() functions
         OCR1A = abs((int)value - 1000) * DIV_CLOCK;
         if(value > 1040) {
             a_bottom_off;
@@ -451,7 +472,11 @@ void position(uint16_t value)
             a_bottom_off;
             b_bottom_off;
         }            
-    } else {
+}
+#elif DRIVER==DRIVER_HBRIDGE
+void position(uint16_t value)
+{
+    lastpos = value;
         uint16_t OCR1A_u = 16000, OCR1B_u = 16000, ICR1_u = 1000;
         // use 62.5 hz at full power to reduce losses
         // some cycling is required to refresh the bootstrap capacitor
@@ -515,8 +540,10 @@ void position(uint16_t value)
                 sei();
             }
         }
-    }
 }
+#else
+#  error unsupported value for DRIVER
+#endif
 
 uint16_t command_value = 1000;
 void stop()
@@ -577,19 +604,20 @@ void disengage()
     digitalWrite(led_pin, LOW); // status LED
 }
 
 void detach()
 {
-    if(pwm_style) {
+#if DRIVER!=DRIVER_HBRIDGE
+#  error __ not HBRIDGE
         TCCR1A=0;
         TCCR1B=0;
         while(digitalRead(pwm_output_pin)); // wait for end of pwm if pulse is high
         TIMSK1 = 0;
-        if(pwm_style == 2) {
+#  if DRIVER==DRIVER_VNH2SP30
             a_bottom_off;
             b_bottom_off;
             digitalWrite(enable_pin, LOW);
-        }
-    } else {
+#  endif
+#elif DRIVER==DRIVER_HBRIDGE
         TCCR1A=0;
         TCCR1B=0;
         TIMSK1 = 0;
@@ -597,7 +637,9 @@ void detach()
         a_bottom_off;
         b_top_off;
         b_bottom_off;
-    }
+#else
+#  error unsupported value for DRIVER
+#endif
     TIMSK2 = 0;
     timeout = 128/DIV_CLOCK; // avoid overflow
 }
@@ -609,14 +651,14 @@ void engage()
         return;
     }
 
-    if(pwm_style == 1) {
+#if DRIVER==DRIVER_RC_PWM
         TCNT1 = 0x1fff;
         //Configure TIMER1
         TCCR1A=_BV(COM1A1)|_BV(WGM11);        //NON Inverted PWM
         TCCR1B=_BV(WGM13)|_BV(WGM12)|_BV(CS11); //PRESCALER=8 MODE 14(FAST PWM)
         ICR1=40000/DIV_CLOCK;  //fPWM=50Hz (Period = 20ms Standard).
         TIMSK1 = 0;
-    } else if(pwm_style == 2) {
+#elif DRIVER==DRIVER_VNH2SP30
         TCNT1 = 0;
         TCCR1A=_BV(COM1A1)|_BV(WGM11);        //NON Inverted PWM
         TCCR1B=_BV(WGM13)|_BV(WGM12)|_BV(CS10); //PRESCALER=0 MODE 14(FAST PWM)
@@ -636,7 +678,7 @@ void engage()
         pinMode(enable_pin, INPUT);
 
         digitalWrite(enable_pin, HIGH);
-    } else {
+#elif DRIVER==DRIVER_HBRIDGE
         //Configure TIMER1
         TIMSK1 = 0;
         TCNT1 = 0;
@@ -653,7 +695,9 @@ void engage()
         pinMode(hbridge_b_bottom_pin, OUTPUT);
         pinMode(hbridge_a_top_pin, OUTPUT);
         pinMode(hbridge_b_top_pin, OUTPUT);
-    }
+#else
+#  error unsupported value for DRIVER
+#endif
 
     position(1000);
     digitalWrite(clutch_pin, HIGH); // clutch
@@ -688,10 +732,11 @@ uint8_t adc_cnt;
 // has 6862 samples/s
 ISR(ADC_vect)
 {
-    if(pwm_style)
-        ADCSRA |= _BV(ADSC); // enable conversion
-    else
+#if DRIVER==DRIVER_HBRIDGE
         sei(); // enable nested interrupts to ensure correct operation
+#else
+        ADCSRA |= _BV(ADSC); // enable conversion
+#endif
 
     uint16_t adcw = ADCW;
     if(++adc_cnt <= 3) // discard first few readings after changing channel
@@ -733,8 +778,9 @@ ISR(ADC_vect)
 #endif
     ADMUX = adcref | muxes[adc_counter];
 ret:;
-    if(!pwm_style)
+#if DRIVER==DRIVER_HBRIDGE
         ADCSRA |= _BV(ADSC); // enable conversion
+#endif
 }
 
 uint16_t CountADC(uint8_t index, uint8_t p)
@@ -788,8 +834,9 @@ uint16_t TakeAmps(uint8_t p)
 {
     uint32_t v = TakeADC(CURRENT, p);
 
-    if(pwm_style == 2) // VNH2SP30
-        return v * 9 / 34 / 16;
+#if DRIVER==DRIVER_VNH2SP30
+    return v * 9 / 34 / 16;
+#endif
     
     if(low_current) {
     // current units of 10mA

@McNugget6750
Copy link
Author

McNugget6750 commented Nov 26, 2019

@CapnKernel Hey Mitch, I'm too far along now. I completely agree that double work is not a good idea and the code base should always be maintained by one source but I invested a couple days into understanding the code now and I would like to at least get this to work.
To make this simpler for everyone, I forked and uploaded my current changes to my repository: https://github.com/McNugget6750/pypilot/tree/master/arduino/motor

I like to split out config.h and pins.h as it brings a lot of order into the chaos.

@seandepagnier What rudder values does pypilot expect? I'm sending ADCs over to pypilot but the rudder position only changes between -94 and -88. It's a 10 bit value that I - like you - multiplied by 4. Then I just send out the raw result.
However, I didn't find a good way to look at what pypilot actually receives except the scope. But the scope seems to be a processed value and not the raw data.

How can I change that? How is the the rudder sensor calibration supposed to work?

@McNugget6750
Copy link
Author

Got it. Rudder is a value from 0 to 65535 with 32768 being the center at 0 degrees.
Since I could not find a configuration for this under Client when coming from Autopilot Control in OpenPlotter, I will make the analog MIN/MAX values a configuration option in config.h. However, this should really come from autopilot and be stored in EEPROM. Maybe it's already in there but I can't seem to find it.
I still need to filter the rudder sensor values. Right now, they come through as raw as they can be which is not desirable.

Next stop is servo control. The PWM stuff should be working (untested) but when I look at the raw values that autopilot sends out (looking at the Scope) they don't make sense to me, yet. The values expected by the firmware range from 0 to 2000. However, what the scope shows when I move the IMU is 0.8 and -0.8.

@CapnKernel
Copy link

@CapnKernel Hey Mitch, I'm too far along now.
I invested a couple days into understanding the code now and I would like to at least get this to work.

@McNugget6750, I think I'd say the same in your situation :-)

I like to split out config.h and pins.h as it brings a lot of order into the chaos.

I had thought of doing something like that as a v2, something somewhat kernel-like, that is to say, a .config file from which config.h can be generated. Is that what you're thinking?

To make this simpler for everyone, I forked and uploaded my current changes to my repository: https://github.com/McNugget6750/pypilot/tree/master/arduino/motor

How far along is your development of this idea? Ie, is this aspect of it working now, in the code that's been pushed to your repo?

Looking forward to working with you.

@CapnKernel
Copy link

CapnKernel commented Nov 27, 2019

It looks like the rudder sensor gives absolute values while the command coming from PyPilot is a desired PWM with the center being 1000? Like an RC servo.

Yes I'm confused by this too. I understand that 1000 is the idle value, but I can't work out whether the number corresponds to some notion of where the rudder is, or whether it's how fast the rudder should be moving (the difference from 1000 being the velocity). Is it that the value was originally that of RC servo position, and the meaning has somewhat changed? @seandepagnier, can you help me understand how this value maps to drivers which aren't an RC servo please?

  1. The engage() function always first resets position(1000); instead of ramping from one value to the next. Wouldn't that cause a lot of jerk? Since every single time a new COMMAND_CODE is processed in void process_packet() the function engage() is called.

void position() takes in value and (in case of pwm_style == 2) drives the enable OI on the vnh2sp30 either CCW or CW and that's it!
What is "controlling" on the rudder position? Is that PyPilot? Because it looks that way.
My understanding is that the rudder position is reported back to PyPilot and PyPilot then sends out a desired PWM value between 0 and 2000 to either just drive the motor CCW or CW. PyPilot maintains complete control over the PID (or similar) controller and motor.ino only really drives PWM and reports telemetry. Is that understanding correct?

Good questions. I'd also like to understand this.

@McNugget6750
Copy link
Author

It seems communication is working. I get the rudder position report in Autopilot and can engage.
Please check it out. If you don't insist on using 16khz PWM, it should already spit out 1khz PWM for the IBT_2. However, I have not tested this.
I also commented everything telemetry out except the rudder sensor. It's kind of a fresh start. I'll bring temperature, voltage and current back in once I get motion on the rudder from my hydraulic pump.

@CapnKernel
Copy link

CapnKernel commented Nov 27, 2019

@CapnKernel Hey Mitch, I'm too far along now.
I invested a couple days into understanding the code now and I would like to at least get this to work.

Well sure, but working code trumps work in progress.

I've had a look at your changes, and it's something I can conceptually work with. But it breaks support for the existing drivers, mostly because lots of the old code is commented out. I do very much like the "explorer's notes" comments you've put into the code. Question: Do you intend for your work to be incorporated back into pypilot soon? (Or were you looking to make a permanent fork?)

my current changes to my repository:
https://github.com/McNugget6750/pypilot/tree/master/arduino/motor

I'm tempted to selectively take some of the work you've done, the parts that don't break the current support for drivers, and put those changes into my own fork, with the intention of passing them back to pypilot. Thoughts?

@seandepagnier, it would be good to get your feelings on this.

Mitch.

@CapnKernel
Copy link

I have bought a small servo and I'll try out my code on it soon.

@McNugget6750
Copy link
Author

Generally, I think that's a good idea.

However, to be honest: Keeping the original code alive with support for a bare metal H-bridge with essentially no self protection seems a bit like keeping code for a floppy disk drive alive even though no one will ever use it again. The spectrum of off the shelve h-bridge hardware is so large and this code is very complex only because of obsolete hardware.
My thinking is, show me one person who actually drives MOSFETs with this code instead of the vnh2sp30 or the IBT_2. I don't think that will ever happen again. Plus, there are no wiring diagrams, no schematics, no parts lists. If there were, I would not have started digging through the code in the first place. For these reasons, I strongly recommend calling this version 2 and starting by creating a new driver structure like it's being done for 3D printers for years. The Marlin 3d printer firmware is an amazing example of configurability and compatibility for different hardware.

@McNugget6750
Copy link
Author

I've had a look at your changes, and it's something I can conceptually work with. But it breaks support for the existing drivers, mostly because lots of the old code is commented out. I do very much like the "explorer's notes" comments you've put into the code. Question: Do you intend for your work to be incorporated back into pypilot soon? (Or were you looking to make a permanent fork?)

I would love for this to go back into public domain and therefore be available to PyPilot users. However, if I have to, I would go and leave this a permanent fork. But I'd rather integrate it back into the PyPilot.

@McNugget6750
Copy link
Author

@seandepagnier I hope you don't get my comments and questions the wrong way! Without you, all this wouldn't exist!
I am passionate about this, because I care(!) and because I also want to use it!

@CapnKernel
Copy link

Hi Timo,

Keeping the original code alive with support for a bare metal H-bridge with essentially no self protection seems a bit like keeping code for a floppy disk drive alive
show me one person who actually drives MOSFETs with this code instead of the vnh2sp30 or the IBT_2

I agree, but I will wait for @seandepagnier's thoughts.

@seandepagnier I hope you don't get my comments and questions the wrong way! Without you, all this wouldn't exist!
I am passionate about this, because I care(!) and because I also want to use it!

I feel the same.

I would love for this to go back into public domain

pypilot.org says that PyPilot is covered by the GPL3 license (and see for example, the top of motor.ino. Because you've based your work on PyPilot, your work can't go into the public domain (and this is, IMO, a good thing).

@rgleason
Copy link

The point of Sean's pypilot was to be very efficient, use of energy and reduce unnecessary movement.

Taking a meat cleaver to it will change that advantage. IMHO

@rgleason
Copy link

Keeping the original code alive with support for a bare metal H-bridge with essentially no self protection seems a bit like keeping code for a floppy disk drive alive

I dont think you understand what Sean has done here.

@partyvi
Copy link
Contributor

partyvi commented Nov 27, 2019

Side view - looks like you guys are doing right thing, but it is going little far from original code means it is not the seandepagnier code any more. Nobody keep us from make ‘right’ motorV2 implementation and put it next to motor.ino directory and have Two versions of the motor controllers. Or just keep in your repository and MAINTAIN it, if it is better people will use it.

I respect seandepagnier and his effort to design and MAINTAIN WORKING pypilot, every code has its history, and when you see from the side YOU THINK you can do it better, so, do it better, share with people. The best part of current motor.ino code IT IS WORKING, AND WORK STABLE.

NOTHIG PERSONAL, JUST THOUGHTS IN LOUD

@McNugget6750
Copy link
Author

McNugget6750 commented Nov 27, 2019

@rgleason you are right. I'm sure I haven't understood what @seandepagnier has done, yet. It's an immense amount of work he put into this and I came in and could not find a starting point. As you can see from @CapnKernel and my questions, we're far from understanding how the complete system works. I'm just concentrating on the motor driver and the hope is to make it more modular and more accessible in case people need to add hardware support.
Having said that, now that I have a better understanding of what's going on and there is at least one guy who has gotten the IBT_2 to work using almost the original code, I'm kinda hopeful to be able to do the same.

@partyvi I agree with you. It's a well known principle: "Not invented here" hits basically everyone and I'm well aware of it. It basically means that one developer very easily falls into the trap of saying another one's code/product is bad - just because he did not invent it himself.
That's why I'm trying to have a conversation instead of just ripping everything apart. It's always difficult to wrap one's head around something someone else already spend months on.

@seandepagnier
Copy link
Member

I completely understand the need to modify and tweak my code to suit your needs as the resulting software will remain GPL!

I would like to support configuring the driver at compile time, however with DRIVER==GENERIC meaning to use the current io strapping arrangement so it works on existing hardware.

Other drivers can use these io pins for other arrangements or purposes.

As far as driving h bridge directly instead of using a module: What is the cost and on resistance of your driver? We should compare these factors when considering what is obsolete.

Don't get too caught up in motor.ino: the future drivers will be brushless anyway.

@McNugget6750
Copy link
Author

McNugget6750 commented Nov 27, 2019

The point of Sean's pypilot was to be very efficient, use of energy and reduce unnecessary movement.

Taking a meat cleaver to it will change that advantage. IMHO

This piece I really haven't understood, yet. The power savings options.
The AVR doesn't consume much energy at all. In comparison with any motor driving the rudder, it's negligible. The Arduino Nano often used for this project has four LEDs on it. They consume 160mA when lit. Just the communication chip on the back of the board consumes 15-20 mA or so. The AVR is highly efficient. By removing the LEDs, I got my power consumption down to 25mA in a different project. Downclocking to 4Mhz instead of the regular 16Mhz will not have such a big impact in power consumption. Unless you are driving the entire system with a set of AA batteries, you probably don't need that.
The movement is dictated by PyPilot, not the motor driver.

@McNugget6750
Copy link
Author

I completely understand the need to modify and tweak my code to suit your needs as the resulting software will remain GPL!

I would like to support configuring the driver at compile time, however with DRIVER==GENERIC meaning to use the current io strapping arrangement so it works on existing hardware.

Other drivers can use these io pins for other arrangements or purposes.

As far as driving h bridge directly instead of using a module: What is the cost and on resistance of your driver? We should compare these factors when considering what is obsolete.

Don't get too caught up in motor.ino: the future drivers will be brushless anyway.

I apologize, I misspoke: I meant open source, not public domain. I realize there are legal differences between GPL and public domain.

Sean, in order to either stop me from building my own version of the motor controller and to help others getting started easier, I think it makes sense to create documentation for your original code.
I propose at least a block diagram or schematic showing all the sensors and where they should be connected to. All the jumper flags. All resistors and other parts needed. All connections made for h-bridges and current sensing.
Then, as a second step, we should document the code and point people who need to make modifications to the right sections in the code.
While doing that, I would like to propose renaming a couple things as you have already started a couple days ago. Can we start a google doc or a draw.io document to be able to work on this together with this group?

Regarding brushless: I'm all for it. However, the driver motor is something a lot of people might already have. I am building this to drive a 35 year old hydraulic servo pump on my Uniflight 42. Going brushless only really works on scratch builds. No?

@seandepagnier
Copy link
Member

Reducing the cpu speed to 4mhz was done not for power saving but for improved analog sampling of the current feedback. A side benefit is it does save a few milliamps. The voltage regulator and as mentioned LEDS waste a lot of power. I have a new version of hardware to run the avr at 3.3v and cut idle current down hopefully to 4-5mA from 15mA. Why does it matter? Because you could leave the controller powered for weeks not using it, it should not be an energy waste.

There are schematics available already:
http://pypilot.org/schematics/

If there are several supported schematics can we post links to them for each driver?

As for commenting and renaming the code, I don't see any problems as long as no functional changes are made. Changing something can inadvertently cause unexplained behavior elsewhere, and as mentioned before this code seems to be "stable" Reordering a few instructions and it would not be.

As for brushless controllers, they can already drive brushed motors which is supported in a lot of software so this is really the future controller. I went with brushed because of all the existing motors, the electronics are slightly cheaper and the controller logic much simpler.

@McNugget6750
Copy link
Author

@seandepagnier Hey Sean, I'm trying to register in the forum (http://forum.openmarine.net/) but for days I have been waiting for several activation emails. Since you're a MOD, could you please do me a favor and take a look? My username is McNugget. I'll delete this message after.

@sailoog
Copy link
Contributor

sailoog commented Nov 29, 2019

@seandepagnier Hey Sean, I'm trying to register in the forum (http://forum.openmarine.net/) but for days I have been waiting for several activation emails. Since you're a MOD, could you please do me a favor and take a look? My username is McNugget. I'll delete this message after.

Done, try now.

@McNugget6750
Copy link
Author

Excellent, thank you!

@McNugget6750
Copy link
Author

Motor control works and rudder feedback works!
Filtering of ADC inputs is further down on the list.
Next up is getting all the other sensors to work that @seandepagnier implemented:

  • Temperature of the H-Bridge
  • Temperature of the motor
  • Motor current
  • Supply Voltage

Since the code is not using any timers manually, I was able to quickly integrate a TFT display library to give me some simple debugging interface as well.

I must admit, 1khz PWM is quite beepy. Maybe we call this a power boat branch and not a sail across the Atlantic branch. When an engine is running, I won't be able to hear it. Under sails, that's a different question.

@McNugget6750
Copy link
Author

Since the update to OpenPlotter 1.2 Alpha the rudder value that's being displayed in the scope only goes from -45 to +45. Before, it went from -100 to +100. Did that change in OP? Did I change something and didn't realize?

@McNugget6750
Copy link
Author

@sailoog I can view the forum now but when I post an issue (GPS baud rates > 4800 seem to crash the GPS and sometimes the entire serial monitor), my post never shows up. Is the post waiting for moderator approval?

@sailoog
Copy link
Contributor

sailoog commented Dec 2, 2019

@sailoog I can view the forum now but when I post an issue (GPS baud rates > 4800 seem to crash the GPS and sometimes the entire serial monitor), my post never shows up. Is the post waiting for moderator approval?

Yes, the first post is always moderated

@McNugget6750
Copy link
Author

The following is working at the moment:

  • Supply Voltage
  • Rudder Position (even though I haven't understood how calibration is supposed to work, yet)
  • Controller and motor temperature
  • General communication (however, going over RS232 as well as UART causes intermittent issues, over USB directly into the Arduino it works well. Should be a physical issue)
  • Debugging / status display
  • Hydraulic motor control using an off the shelf IBT_2 full h-bridge module

I will have to do some cleanup now and make the display something you can use as a status display all the time.
You will notice in the picture that I tried using a MAX232 to convert to RS232 but one of the two got really hot and the connection was not made. I assume it's broken and it needs to be replaced. I was hoping the aforementioned communication issues are related to TTL level uart through a long, unshielded cable and I can mitigate this issue by using two MAX232. However, so far I had no luck with that.
IMG_0165
IMG_0168

@CapnKernel
Copy link

CapnKernel commented Dec 5, 2019 via email

@McNugget6750
Copy link
Author

The last changes I saw, made by @seandepagnier, were only refactoring a few items as far as I recall. If you want to check out my modifications, it makes sense to do it without me pulling changes.

@McNugget6750
Copy link
Author

@CapnKernel I am reading up on pulling changes from the parent repository into my fork. I haven't done this before and since I made extensive changes to the code, I am concerned it will break something in the process. That's why I'm a bit reluctant :)

@McNugget6750
Copy link
Author

I merged the changes. If you test the code and find issues, please post them in the other repository. I have been spamming this "issue" enough.

@McNugget6750
Copy link
Author

@seandepagnier Two questions:

  • Rudder sensor feedback > 32768: rudder is further PORT or STARBOARD?
  • motor command to motor.ino >1000: move rudder towards PORT or STARBOARD?

@seandepagnier
Copy link
Member

seandepagnier commented Dec 7, 2019 via email

@CapnKernel
Copy link

CapnKernel commented Dec 8, 2019 via email

@McNugget6750
Copy link
Author

i’m not sure why, but during my test run that I’m conducting right now, the auto pilot is steering in the wrong direction. When it should turn right steers left. When it should steer left it steers right. however, when I use the buttons on the pypilot GUI port and starboard is correct. How can I change this behavior?

@McNugget6750
Copy link
Author

McNugget6750 commented Dec 9, 2019

For now, I have swapped the behavior for port and starboard on the motor controller side. That seems to work. But that means the buttons in the gui are reversered for pure manual control.

EDIT: Course following works and I even had the chance to tune the PID controller! I'm extremely excited right now! However, I really want to get the GPS route waypoint following to work as stated here: http://forum.openmarine.net/showthread.php?tid=2134

@seandepagnier
Copy link
Member

seandepagnier commented Dec 9, 2019 via email

@McNugget6750
Copy link
Author

McNugget6750 commented Dec 9, 2019

In this gui, started by OpenCPN the buttons for port and starboard are now reversed:
image

But when I engage the autopilot in either compass or GPS mode, the buttons to change course are now working!
image

However, now that I can keep a course with reasonable precision, I really want to get waypoint following to work. I can't seem to find the setting that allows openCPN to communicate the next waypoint to pypilot.
What are the steps I need to take to enable waypoint following?

@McNugget6750
Copy link
Author

If I go through OpenPlotter, go to pypilot and then calibration and then rudder to get to this window:
image
I cannot set a negative gain for the rudder. Where do you mean? Where can I set a negative gain?

@seandepagnier
Copy link
Member

seandepagnier commented Dec 9, 2019 via email

@McNugget6750
Copy link
Author

McNugget6750 commented Dec 9, 2019

That seems to be the secret ingredient that I can’t figure out. I know where to set it up but I don’t know what pypilot’s IP is. If the connection is set up correctly and the route is active, I should see APB messages in the NMEA debug window, correct?

@seandepagnier
Copy link
Member

seandepagnier commented Dec 9, 2019 via email

@McNugget6750
Copy link
Author

McNugget6750 commented Dec 9, 2019

I fear you mean what I needed to set up under connections. There I put localhost there and it works. But it’s only set up as an input.
Then, based on this other success report, I tried 192.168.14.1 as well as 10.10.10.1 with input and output and both didn’t work:
5F385122-E4BA-41CA-9A0C-3FFFC332BCC6

Are you saying, I should just set localhost up as input AND output and that’s it?

EDIT: I guess, following your previous comment, I need to set localhost as I/O at port 20200. I’ll try that tomorrow and test drive it next weekend.

@seandepagnier
Copy link
Member

seandepagnier commented Dec 9, 2019 via email

@McNugget6750
Copy link
Author

McNugget6750 commented Dec 11, 2019

Going through the steps now:

These messages after I added an outgoing TCP connection:
image

Seems like Pypilot is running under 10.10.10.1
image

Changing the outgoing TCP connection to that address didn't help.

Here is a netstat -l:
image
0.0.0.0:20200 as an IP and port makes little sense.

As a reminder, when I installed all this, I took the vanilla OpenPlotter ISO that can be downloaded with everything pre-configured. That's why I am still searching through menus and configs to figure out where this is even set up? I don't know how to configure the IP pypilot is using. Can you please point me to the correct config or menu where I can see that?

@McNugget6750
Copy link
Author

Everything works!

I apologize for my brain fart regarding the IP addresses. Obviously, because everything runs on the Raspberry Pi, localhost does it all. Also, I flipped a bit on the port number, which is 20220 and not 20200.
After fixing that and only sending the required NMEA messages, pypilot woke up and follows waypoints like a champ! I’m still tuning the controller a little but overall I’m extremely happy with the result!
I will now document my steps and publish what I have put together.

Thank you all for the help!

@CapnKernel
Copy link

CapnKernel commented Feb 6, 2020

i’m not sure why, but during my test run that I’m conducting right now, the auto pilot is steering in the wrong direction. When it should turn right steers left. When it should steer left it steers right. however, when I use the buttons on the pypilot GUI port and starboard is correct. How can I change this behavior?

I found the same thing: Either I can have the left button in standby mode turn the boat to port, but in autopilot mode, PyPilot just drives the boat further and further off course, or by switching the wires, the autopilot works but the left and right buttons work backwards, ie, not useful.

@McNugget6750, did you find the cause?

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

No branches or pull requests

6 participants