descampsa / ardyno

Arduino library for dynamixel servos
BSD 2-Clause "Simplified" License
40 stars 20 forks source link

Incorrect position reporting and strange reset behavior #10

Closed eecharlie closed 7 years ago

eecharlie commented 7 years ago

Here is a minimal example that reveals 2 problems: 1) When I read the motor position, it's always returned as 0. 2) When I run this code that simply sets the goal position and then queries it, it resets the Arduino after a few iterations through the main loop.

Note that I'm using a MEGA ADK and Serial1 as the AX-12 interface. I am using an external power supply in addition to USB to the Arduino, so even if the AX-12 drew significant current, it shouldn't be able to brown-out the Arduino.

I should mention that the motor does move roughly consistently with the goal commands, so it's not simply a total communication failure. I've also included an ISR trap to catch any uncaught interrupts, but it hasn't been triggered.

An even simpler version of this code that never sends a goalPosition() command, just calls currentPosition(), also crashes after a few iterations.

Typical Output:

Starting...
0 target/actual 0
10 target/actual 0
20 target/actual 0
30 target/actual 0
40 target/actual 0
Starting...
Starting...
0 target/actual Starting...
Starting...

Code:

#include "DynamixelMotor.h"
#include <SoftwareSerial.h>

const uint8_t id=1;
int16_t pos=0;

HardwareDynamixelInterface interface(Serial1);
// Use this if you use a direction pin with 3-state buffer
//HardwareDynamixelInterface interface(Serial, 2);
// Use this if you want a sofware serial interface (communication speed will be limited)
//SoftwareDynamixelInterface interface(2,3);

DynamixelMotor motor(interface, id);

//http://stackoverflow.com/questions/23377948/arduino-avr-atmega-microcontroller-random-resets-jumps-or-variable-data-corrup
ISR(BADISR_vect)
{
    for (;;) UDR0='!';
}

void setup()
{ 
  Serial.begin(57600);
  delay(100);
  Serial.println("Starting...");

  interface.begin(1000000);
  delay(100);

  motor.init();
  motor.enableTorque();

  // reset to 0 position
  motor.jointMode();
  motor.speed(256);
  motor.goalPosition(pos);
  delay(1000);
}

void loop() 
{
  motor.goalPosition(pos);
  Serial.print(pos);
  Serial.print(" target/actual ");
  Serial.println(motor.currentPosition());
  pos+=20;
  if (pos >= 1000) { pos = 0; }
  delay(1000);
}
eecharlie commented 7 years ago

Update: I think I have found the problem, but not sure best approach to solve.

The Serial1 port (in this case the AX-12 interface), with no hardware tristate buffer to prevent it writing data to itself, is filling its input buffer with junk anytime a command is issued. Or something similar.

When I use

while(Serial1.available()) { Serial1.read(); } 

just before a read operation like motor.currentPosition() suddenly things start working. If I leave it with literally just that then things work, but if I try to echo these chars to Serial.print(), things get unstable. Should something be built into the library that automatically clears the input buffer as needed?

Is there a wiki somewhere for all of these tricks? I am having to learn far more things than I care to remember :)

descampsa commented 7 years ago

I have tested with a Uno, with Serial as Dynamixel interface and led as feedback, and it seems to work as expected. I don't have a Mega to test with right now, unfortunately.

The library disables the UART receiver when it transmits, and the transmitter when it receives, so there shouldn't be anything received when you send a packed. Either this part of the code doesn't work correctly for some reason on the atmega256, or you have some other junk source on your data lines. I will check the atmega 256 datasheet.

The resets make me think that there is something wrong in your setup. Have you changed the fuse configuration somehow? Also make sure that you don't have a watchdog active.

descampsa commented 7 years ago

I have found and fixed an error in code that might be related, the receiver interrupt was enabled even in transmit mode. I don't think it should make a difference, since the receiver itself was correctly disabled. To be sure, could you test with the last github version to see if it makes a difference?

eecharlie commented 7 years ago

I'm happy to pull the last github version and test. I will be able to do this Monday, and I'm on UTC-7.

I'd be surprised if it was a watchdog because I added the BAD ISR vector to write "!" to the UART for uncaught interrupts. However I did not double-check that the UART address is correct for the mega.

Have you considered using unit tests to help identify problems and validate solutions for situations like this? There is an ArduinoUnit 2.0 library that's quite good for this. Because your library interfaces to hardware, I'd probably do a couple test suites that have different requirements for whether and how there are AX-12's attached during the test. For this particular problem, I'd call a representative set of commands that cause a read/write and then verify that SerialX.available() returns 0 after each.

Would it help if I set up an example of this?

descampsa commented 7 years ago

I'd be surprised if it was a watchdog because I added the BAD ISR vector to write "!" to the UART for uncaught interrupts.

If i read the datasheet correctly, the watchdog can be configured to do a global reset instead of calling an interrupt, and in this case, your BAD ISR interrupt function will not be called. I am not convinced either that this is the cause of your problem, but it is worth a check.

It would be interesting to see the 'junk' data received by the uart, to know if is the packet sent, or something else. If it is something else,i would try to add a pullup resistance on the data line (something like 10K), maybe it is picking up some external noise when it is in floating state.

Have you considered using unit tests to help identify problems and validate solutions for situations like this?

Not yet, but it would probably be usefull, yes, even if it impossible to cover all cases when hardware is involved. If you have an example or can make one , it would certainly be welcome.

eecharlie commented 7 years ago

I've forked the branch and implemented a test that partly works but then fails due to the Serial1 buffer having data that it shouldn't. Would you like me to submit a pull request?

descampsa commented 7 years ago

Thanks, i will have a look at it. What result do you get when you run it, exactly?

eecharlie commented 7 years ago

Exact results (specific character count and values is not always consistent):

Test 0_dummy passed.
400 target/actual 533
410 target/actual 529
420 target/actual 511
430 target/actual 495
440 target/actual 487
450 target/actual 482
460 target/actual 472
470 target/actual 464
480 target/actual 464
490 target/actual 478
500 target/actual 494
510 target/actual 506
520 target/actual 511
530 target/actual 518
540 target/actual 529
550 target/actual 541
560 target/actual 553
570 target/actual 561
580 target/actual 569
590 target/actual 580

Repeating with increased delay

400 target/actual 520
410 target/actual 440
420 target/actual 422
430 target/actual 433
440 target/actual 442
450 target/actual 454
460 target/actual 463
470 target/actual 472
480 target/actual 482
000
 chars left in Serial1 buffer: 3
Assertion failed: (BufferIsClear()=0) == (true=1), file test_mega_hardware_serial.ino, line 108.
Test 1_junk_in_buffer failed.
Test summary: 1 passed, 1 failed, and 0 skipped, out of 2 test(s).
descampsa commented 7 years ago

I see, this is clearly noise on the line. Could you try to add pinMode(18, INPUT_PULLUP); in the setup and let me know if it solves the problem? That should add a pullup to the data line when it is in receiving mode, instead of letting it floating.

eecharlie commented 7 years ago

The test passes with that added. Should this be included in the interface library then? I imagine that involves a bunch of dependencies on which chip and which port is being used...

descampsa commented 7 years ago

It should definitely be in the library, but i don't know yet how to determine the gpio port number from the serial hardware interface. Maybe i will have to code each case manually...

eecharlie commented 7 years ago

At least one related thread in the forums suggests you can't: http://forum.arduino.cc/index.php?topic=464818.0

The problem is twofold: you need to know which serial object was passed in, and also what CPU is being used. Spurious serial receive events seem like a somewhat common problem due to floating input pins.

I would look for a macro or member object defined in the serial port library, and if it doesn't exist that's where I think it belongs because it's generally useful. I've never attempted to contribute to Arduino core code. A serialX.RXPIN or similar would be perfect.

In the meantime a call-out in your read me and an example would be an improvement without requiring you to sort out all the CPU specific #ifdef conditions.

descampsa commented 7 years ago

It is apparently not present in the Arduino library, so i have implemented it myself for most avr board. Could you try to run your test (without the pinMode(18, INPUT_PULLUP);) with the last github version, and confirm if it fix the problem for you? Thanks

eecharlie commented 7 years ago

Sure. Do you want to execute the pull request and then I can merge your master?

eecharlie commented 7 years ago

Thanks. Do you know of a good simple test of write and read-back to confirm connectivity? For example read back the register/RAM containing LED state?

I'm out of my office for the weekend and running the test remotely failed, but I may or may not have left the 12v plugged in :) also this would be a good first unit test to troubleshoot wiring.

descampsa commented 7 years ago

I have added a simple test that check the communication, using the led register as you proposed.

eecharlie commented 7 years ago

Great. All of the unit tests are passing for me now (the problem I encountered this weekend was a mismatch of the motor ID).

descampsa commented 7 years ago

Ok, perfect, thanks a lot for reporting the problem and helping me to fix it!

eecharlie commented 7 years ago

You are welcome, thank you for being so responsive in fixing the library to meet my application needs!

And you can see how useful unit tests are. I encourage you to learn about and consider test-driven development, which means always writing a test before writing the feature that must pass it. It is hard to get used to but I find that it always saves time even though it doesn't feel like it at first. And while it may seem unreasonable for systems that interface to hardware, it's not, here's a good reference:

https://pragprog.com/book/jgade/test-driven-development-for-embedded-c

and look up cmocka, a much newer and advanced tool though unfortunately isn't for avr-libc or for c++