EFeru / hoverboard-firmware-hack-FOC

With Field Oriented Control (FOC)
GNU General Public License v3.0
1.12k stars 926 forks source link

VARIANT_USART issues :-/ #65

Closed RoboDurden closed 4 years ago

RoboDurden commented 4 years ago

First, i am very happy that your hoverserial.ino worked right out of the box :-)

But i had some issues that forced me to fork your nice firmeware and implement the serial protocol of my previous fork. If you want you can search for SERIAL_ROBO in the following files:

https://github.com/RoboDurden/hoverboard-firmware-hack-FOC/blob/master/Src/main.c https://github.com/RoboDurden/hoverboard-firmware-hack-FOC/blob/master/Inc/util.h https://github.com/RoboDurden/hoverboard-firmware-hack-FOC/blob/master/Src/util.c

1. HoverReceive() only works fine when the loop() is empty. That is because you set the send-feedback interval to 10 ms in main.c and in HoverReceive() only reads a single byte from the serial buffer.

my HoverReceive() waits until SerialFeedback data is complete and then processes it in one call:

boolean HoverReceive()
{
  if (oHoverSerial.available()<  sizeof(SerialFeedback))
    return false;

  SerialFeedback oNew;
  byte* p = (byte*)&oNew;
  for (unsigned int i=0; i < sizeof(SerialFeedback); i++)
    *p++ = oHoverSerial.read();;

  uint32_t crc = 0;
  crc32((const void *)&oNew, sizeof(SerialFeedback)-4,   &crc);

  if (oNew.crc == crc)
  {
    memcpy(&oFeedback,&oNew,sizeof(SerialFeedback));
    return true;    
  }
  return false;
}

2. Sending a Serialcommand with steer=0 at 9600 baud sometimes yields -1000 for steer :-( I can upload a video where my two wheels start spinning in opposite directions :-(( And your SerialFeedback logs steer = -1000 back !!

I think that is because the stm32 loop is way faster than 9600 baud and it happens more often that readCommand() gets processed while that tiny circular command buffer gets overwritten. Your 0xABCD does not really solve that issue ! Best would be to simply switch from a circular buffer to one that skips incomming data when full. With only 8 bytes and even 10 ms a 1 second buffer would only need 800 bytes. I tried for such a non-circular buffer already with my old fork but i am an absolute stm32 beginner and failed.

My way simply is to make a double copy of the command buffer and than try all 8 offsets until my more secure crc32 is correct.

3. After you succeed with your 0xABCD and the simple crc you CLAMP steer and speed to -1000 +1000. I would call this as a bug. As your 0xABCD and crc is not 100% safe, the chance that a corrupted steer value with a range of 65536 is clamped to +-1000 is very high.

I would at least suggest you validate the speed and steer like if ( (pCmd->steer >= INPUT_MIN) && (pCmd->steer <= INPUT_MAX) && (pCmd->speed >= INPUT_MIN) && (pCmd->speed <= INPUT_MAX) )

4. A faulty steer or speed should not cause damage because of the low-pass in main.c However the mixer is after the low-pass. And a steer change can affect the speed change more then a steer change by itself. But i think that is okay :-)

I have put an arduino example for both protocols to my fork: https://github.com/RoboDurden/hoverboard-firmware-hack-FOC/tree/master/hoverserial

5. In my protocal i send back the current of the two motors. I guess that would be

            Feedback.iAmpL = (int16_t)((float)curL_DC / A2BIT_CONV);    // 154 = 15.4 A
            Feedback.iAmpR = (int16_t)((float)curR_DC / A2BIT_CONV);    // 1542 = 154.2 A

where i made these two variables available with

    extern float curL_DC;   // defined and updated in bldc.c
    extern float curR_DC;   // defined and updated in bldc.c

But i only receive 0 values on the arduino side :-/ Maybe you instantly see what i miss :-)

I am happy that you are still active with your firmware and therefore do not really want to continue my own fork. If you agree that your crc is not 100% safe, you might be happy to include some of my code.

I do not really know how github works. So i do not know how to become a contributor. And i would understand if want to keep such a nasty guy like me out of your nice project :-)

roland, the little physicist :-) "you either lose.. or fail - nothing else - nothing in between !" (Robo Durden)

EFeru commented 4 years ago

Give me some time to process the answer for you.

RoboDurden commented 4 years ago

Take your time :-)

RoboDurden commented 4 years ago

in case you don't belive me: https://youtu.be/cpnbJV8CpyU video is not public :-)

RoboDurden commented 4 years ago

Okay i added a nice VARIANT_UARTCAR and ported my closed loop into your main.c. Right after your VARIANT_UARTCAR section, so you might like it:

      #ifdef VARIANT_UARTCAR
        float fmms = WHEEL_SIZE_INCHES * 1.32994089;  // mm/s = (WHEEL_SIZE_INCHES * 25.4 * 3.142) * (rpm / 60)
        long iSpeed;
        if (abs(rtY_Left.n_mot) > abs(rtY_Right.n_mot))
        {
          #ifdef INVERT_L_DIRECTION
            iSpeed = -fmms * rtY_Left.n_mot;
          #else
            iSpeed = fmms * rtY_Left.n_mot;
          #endif
        } 
        else
        {
          #ifdef INVERT_R_DIRECTION
            iSpeed = fmms * rtY_Right.n_mot;
          #else
            iSpeed = -fmms * rtY_Right.n_mot;
          #endif
        } 
        long iSpeed_Goal = (cmd2 * 1000) / 36;  // 10*kmh -> mm/s
        if (    (abs(iSpeed_Goal) < 56) && (abs(cmd2Goal) < 50) )   // iSpeed_Goal = 56 = 0.2 km/h
            speed = cmd2Goal = 0;
      #ifdef MAX_RECUPERATION
        else if ( (float)(curL_DC+curR_DC)/(-2.0*A2BIT_CONV) < MAX_RECUPERATION * -1)
        {
          cmd2Goal += 5;
          if (cmd2Goal > 1000)  cmd2Goal = 1000;
        }
      #endif
        else if (iSpeed > (iSpeed_Goal + 56))   // 28 = 27.777 = 0.1 km/h
        {
          cmd2Goal -= CLAMP((iSpeed-iSpeed_Goal)/56,  1,3);
          if (  (iSpeed_Goal > 56)  && (cmd2Goal < 2)  ) cmd2Goal = 2;   // don't set backward speed when iSpeed_goal is set forwards
          else if (cmd2Goal < -1000)    cmd2Goal = -1000;
        }
        else if (iSpeed < (iSpeed_Goal -56))
        {
          //cmd2Goal += 3;
          cmd2Goal += CLAMP((iSpeed_Goal-iSpeed)/56,  1,3);
          if (  (iSpeed_Goal < -56)  && (cmd2Goal > -2)  ) cmd2Goal = -2;   // don't set forward speed when iSpeed_goal is set backwards
          else if (cmd2Goal > 1000) cmd2Goal = 1000;
        }
        cmd2 = cmd2Goal;
      #endif

Only variable i need is:

#ifdef VARIANT_UARTCAR // ROBO
  int16_t cmd2Goal; // goal speed for VARIANT_UARTCAR
#endif

For the MAX_RECUPERATION i need:

    extern int16_t curL_DC; // defined and updated in bldc.c
    extern int16_t curR_DC; // defined and updated in bldc.c

Took me some time to notice that a negative curL_DC stands for power consumption and a positive value for charging the battery.

I think such a VARIANT_UARTCAR makes sense:

// ############################ VARIANT_UARTCAR SETTINGS ############################
#ifdef VARIANT_UARTCAR  // by Robo Durden
  #undef  CTRL_MOD_REQ
  #define CTRL_MOD_REQ        1         // uartCar has it's own closed loop in main.c
                                        // speed is 10*km/h so 60 would be 6.0 km/h
  #define FILTER              20000     // ignore DEFAULT_FILTER for new closed loop  [-] lower value == softer filter [0, 65535] = [0.0 - 1.0].

  #define WHEEL_SIZE_INCHES 6.5                      

  //#define CONTROL_SERIAL_USART2         // long cable USART2 only 3.3V !!!
  //#define FEEDBACK_SERIAL_USART2        // long cable USART2 only 3.3V !!!
  #define CONTROL_SERIAL_USART3         // short cable USART3 is 5V compatible
  #define FEEDBACK_SERIAL_USART3        // short cable USART3 is 5V compatible

  #define SPEED_COEFFICIENT   16384     //  1.0f
  #define STEER_COEFFICIENT   0         //  0.0f
  // #define INVERT_R_DIRECTION           // Invert rotation of right motor
  // #define INVERT_L_DIRECTION           // Invert rotation of left motor

  #define MAX_RECUPERATION 3.0  //increase gas when more then 3.0 amps go back into the battery
                                // my chain drive is to loose to take more then 3 Amps :-/
#endif

I will embark on a new 500 km journey tomorrow - with your nice FOC control, thanks again :-)

EFeru commented 4 years ago

Maybe I am too late but the UART should be improved significantly now. Where is you journey going to be? Have a nice trip ;)

RoboDurden commented 4 years ago

Great that you improved your serial communication. Will give it at try when i am on the road. My SerialFeedback becomes unusable when the motors are running. Don't know why i have so much noise in my cables. But i remember that your SerialFeedback worked better than mine.

I am not really going anywhere. I simply drive the same circle forever. Hof -> Regensburg -> Nurenberg -> Bamberg -> Hof. This is my 500km loop number 46. It is like the northern half of Bavaria is my house.

"All that you own end up owning you." (Tyler Durden)

EFeru commented 4 years ago
  1. The Arduino example can be refined, but it was not my main goal here. The goal was to give a start if someone is using Arduino. Of course it can be done more elegant with interrupts, but again is not my main goal. So feel free to improve.

By waiting for the full lenght package is also not optimal, because one you lose one byte you are out-of-sync and is hard to get back on track. The best is to use "UART Idle line detection interrupt" to process the UART data, as I did in the main firmware. However, I am not sure if Arduino support Idle line detection at all.

  1. I think this has been fixed with last commit

  2. 0xABCD for sure helps, If the first 16 bits are not ABCD, there is no reason to use the data. It for sure lowers the chances of having bad data. Now, feel free to use more complex CRC algorithms, but for me I think the current solution gives a good balance between complexity and wrong data detection.

Also, CLAM replaced by IN_RANGE => so, fixed. Thanks!

  1. Ok

  2. I am not sure how are you sending curL_DC , curR_DC from mainboard, so, I am not sure if I can help here.

RoboDurden commented 4 years ago
  1. it was a stupid type mismatch where i definend them as float but now i receive your values in the main.c:
    extern int16_t curL_DC; // defined and updated in bldc.c
    extern int16_t curR_DC; // defined and updated in bldc.c
RoboDurden commented 4 years ago
  1. Yes you are right and i should have quoted my full HoverReceive where i empty the buffer at the end of the function: while (oHoverSerial.available()) oHoverSerial.read(); // empty garbage That way the commuication syncs by itself.