Odd behavior when wrapping Encoder inside another class

W

wayneandlayne

New member
Hello everyone,

I have been trying to integrate the PJRC Encoder class inside my own motor control class library, but I've been having some troubles, probably related to interrupts or volatile variables or atomic access of multi-byte variables or something like that. This is with stock Arduino 1.0.5 on Ubuntu 12.04, using a LEGO Mindstorms NXT motor as the encoder inputs.

Code posted here: https://github.com/matthewbeckler/EncoderWrapper

EncoderWrapper.h
Code: 
// Arduino header files
#include <stdint.h>
#include "Arduino.h"

// Library header files
#include <Encoder.h>

class EncoderWrapper
{
    public:
        EncoderWrapper(uint8_t pin1, uint8_t pin2);

        // Read the encoder's current position.
        int32_t getPosition(void);

        // Write the encoder's current position
        void setPosition(int32_t pos);

        // Tracks the position of the motor
        Encoder _encoder;
};

EncoderWrapper.cpp
Code: 
#include "EncoderWrapper.h"

EncoderWrapper::EncoderWrapper(uint8_t pin1, uint8_t pin2):
    _encoder(pin1, pin2)
{
    // Nothing to do here
}

// Read the encoder's current position.
int32_t EncoderWrapper::getPosition(void)
{
    return _encoder.read();
}

// Write the encoder's current position
void EncoderWrapper::setPosition(int32_t pos)
{
    _encoder.write(pos);
}

You can see that this class is very simple. It has a local instance of Encoder (_encoder), which is initialized in the initializer list of the constructor. The getPosition() function is a single-line wrapper around Encoder's read() function.

Here is a quick sketch to demonstrate the issue:
Code: 
#include <Encoder.h>
#include <EncoderWrapper.h>

EncoderWrapper w = EncoderWrapper(3, 4);

void setup(void)
{
  Serial.begin(115200);
}

void loop(void)
{
  Serial.println("-----------------------------");
  Serial.println(w.getPosition(), HEX);
  Serial.println(w._encoder.read(), HEX);
  delay(100);
}

When I run the sketch with an encoder connected to pins 3 and 4 on the Arduino, the second Serial.println() statement prints the correct position value from the Encoder instance. The first Serial.println() statement prints garbage that seems to increment over time? Here's some sample output:

Code: 
-----------------------------
620154
0
-----------------------------
C50154
0
-----------------------------
1270154
0
-----------------------------
12E
1
-----------------------------
1EC0154
C
-----------------------------
24E0154
24
-----------------------------
12E
3F
-----------------------------
12E
5B
-----------------------------
3760154
76
-----------------------------
3D80154
92
-----------------------------
12E
AD
-----------------------------
49D0154
C4
-----------------------------
5000154
D4
-----------------------------
5620154
D8
-----------------------------
5C50154
CD
-----------------------------
12E
B6
-----------------------------
68A0154
93
-----------------------------
12E
6A
-----------------------------
12E
40
-----------------------------
12E
1A
-----------------------------
12E
FFFFFFF8
-----------------------------
12E
FFFFFFD8
-----------------------------
12E
FFFFFFBC
-----------------------------
12E
FFFFFFA4
-----------------------------
12E
FFFFFF94
-----------------------------
12E
FFFFFF8E


So, that's pretty weird. I'm not sure why using the wrapper function produces garbage data, but calling the read() function directly works perfectly. Does anyone have any idea?

Thanks much,
Matthew Beckler
Wayne and Layne, LLC
 
Have you tried moving
_encoder(pin1, pin2)
inside the constructor, something like this?
Code: 
EncoderWrapper::EncoderWrapper(uint8_t pin1, uint8_t pin2)
{
    _encoder = new(Encoder(pin1, pin2));
}

...or similar, however you need to initialize it...

I'm not familiar with the library, but sometimes you need to construct manually due to compiler bugs.

Another and possibly better way, would be:
Code: 
// Arduino header files
#include <stdint.h>
#include "Arduino.h"

// Library header files
#include <Encoder.h>

class EncoderWrapper
{
    public:
        // Tracks the position of the motor
        Encoder _encoder;

        EncoderWrapper(uint8_t pin1, uint8_t pin2):_encoder(pin1, pin2) {};

        // Read the encoder's current position.
        int32_t getPosition(void) {return _encoder.read();};

        // Write the encoder's current position
        void setPosition(int32_t pos){_encoder.write(pos);};

};

And then you can ditch the .cpp file all together :cool:

Furthermore you are possibly using namespaces incorrectly in the cpp file. If you still want to use your original way...
You don't need to do foo::blah(bar)...
Just place a class directive at the top of the entire block of code instead, just like in the header file, like this:

Code: 
class EncoderWrapper
{

 ///write all your methods in here...
}
 
One other note-- you could just subclass it too, which would make things even easier :)
 
Thanks for the replies xxxajk!

I'm working to build an Arduino library to provide a nice interface to control some motors, so I definitely need to keep both the CPP and H file for my library, as there will be lots more code in the wrapper class (this was just the simplest class that demonstrated the issue). I don't think subclassing is the right choice either.

I will try moving the object constructor call inside the wrapper's constructor as you suggested. However I was under the impression that this would result in the object getting initialized with the default empty Encoder() constructor first when the wrapper is constructed, and then replacing that object with the new one when I call new Encoder(pin1, pin2). I believe this double-initialization issue is the reason that c++ added the initializer list feature to the constructors.

Since this is on Arduino, I was trying to avoid dynamic memory management (new, free, malloc, etc), but it would probably work to switch from the wrapper storing Encoder and instead store only a pointer to Encoder, which could be initialized using new inside my constructor. Still, I'm not sure if that would work, but I'll give it a try.

I've never seen the use of "class foo { ... }" inside the .cpp file to contain the method implementations, do you know if that's equivalent to the foo::blah(bar) notation?

I'll try some of these ideas tonight after work and post my results. Thanks for the help, I appreciate it!
 
Update: Using the new operator inside the constructor did not help. The compiler did not accept another usage of "class EncoderWrapper" in the cpp file.

What did work was to move everything into the header file, which is ugly as sin, but I guess since Paul's Encoder library is that way, maybe that's why my library has to be that way too?
 
You must log in or register to reply here.
Back
Top