Encoder library - In Encoder_internal_state_t, why isn't position marked volatile?

wayneandlayne · Sep 8, 2014

In the Encoder.h file for the Encoder library, this struct is defined:

Code:

typedef struct {
        volatile IO_REG_TYPE * pin1_register;
        volatile IO_REG_TYPE * pin2_register;
        IO_REG_TYPE            pin1_bitmask;
        IO_REG_TYPE            pin2_bitmask;
        uint8_t                state;
        int32_t                position;
} Encoder_internal_state_t;

Does anyone know why state and position are not also marked as volatile? These variables are updated inside the interrupt (if you are using interrupt pins) so I'm not sure why they aren't marked volatile. Any ideas?

As an aside, I'm working to incorporate the Encoder library into another Arduino library I'm working on. I have been seeing some odd behavior when using my own wrapper function around encoder.read(), so I decided to look at the disassembly listing for my wrapper function. I'm using Arduino 1.0.5 on Ubuntu 12.04.

Here's my wrapper function, and the relevant details of its use of the Encoder library.

MyClass.h:

Code:

#include "utility/Encoder.h"
class MyClass
{
    public:
        MyClass(uint8_t encA, uint8_t encB);
        // other things here
    private:
        Encoder _encoder;
        // other things here
};

MyClass.cpp:

Code:

MyClass::MyClass(uint8_t encA, uint8_t encB):
    _encoder(encA, encB) // using initializer list to initialize Encoder instance
{}
int32_t MyClass::getPosition(void)
{
    return _encoder.read();
}

Here's the end of the disassembly listing for getPosition. We can see at address 0x322 that the SEI (enable interrupts) instruction occurs BEFORE it copies encoder.position into local registers r22-r25.

Code:

0000031a <L47end>:
     31a:       ac 54           subi    r26, 0x4C       ; 76
     31c:       b0 40           sbci    r27, 0x00       ; 0
     31e:       01 c0           rjmp    .+2             ; 0x322 <L47end+0x8>
        inline int32_t read() {
                if (interrupts_in_use < 2) {
                        noInterrupts();
                        update(&encoder);
                } else {
                        noInterrupts();
     320:       f8 94           cli
                }
                int32_t ret = encoder.position;
                interrupts();
     322:       78 94           sei
     324:       fd 01           movw    r30, r26
     326:       ed 5a           subi    r30, 0xAD       ; 173
     328:       ff 4f           sbci    r31, 0xFF       ; 255
     32a:       60 81           ld      r22, Z
     32c:       71 81           ldd     r23, Z+1        ; 0x01
{
   return _encoder.read();
}
     32e:       82 81           ldd     r24, Z+2        ; 0x02
     330:       93 81           ldd     r25, Z+3        ; 0x03
     332:       08 95           ret

If I edit Encoder.h and mark state and position as volatile in Encoder_internal_state_t, then we get this:

Code:

0000031a <L47end>:
     31a:       ac 54           subi    r26, 0x4C       ; 76
     31c:       b0 40           sbci    r27, 0x00       ; 0
     31e:       01 c0           rjmp    .+2             ; 0x322 <L47end+0x8>
        inline int32_t read() {
                if (interrupts_in_use < 2) {
                        noInterrupts();
                        update(&encoder);
                } else {
                        noInterrupts();
     320:       f8 94           cli
                }
                int32_t ret = encoder.position;
     322:       ad 5a           subi    r26, 0xAD       ; 173
     324:       bf 4f           sbci    r27, 0xFF       ; 255
     326:       2d 91           ld      r18, X+
     328:       3d 91           ld      r19, X+
     32a:       4d 91           ld      r20, X+
     32c:       5c 91           ld      r21, X
                interrupts();
     32e:       78 94           sei
{
   return _encoder.read();
}
     330:       b9 01           movw    r22, r18
     332:       ca 01           movw    r24, r20
     334:       08 95           ret

Here we see that the SEI instruction is properly placed after we load encoder.position into local registers r18-r21.

It seems like these two struct fields should be marked volatile, as they are updated by the interrupt handler. It seems like if the ISR fires while the code is copying encoder.position into local registers, we might be left with invalid data (half from before, half from after interrupt, or similar).

Thanks for your thoughts,
Matthew Beckler
Wayne and Layne, LLC

Encoder library - In Encoder_internal_state_t, why isn't position marked volatile?

wayneandlayne

New member