Lessons learned on the encoder driver

[elsevers]

Pitfall

In a couple of our codebases, we discovered that encoder users are finding it confusing to change the number of pulses per revolution. When a user encounters an encoder that has a different number of bits than the default value in encoder.h, there is a tendency to directly edit this line:

AMDC-Firmware/sdk/app_cpu1/common/drv/encoder.h

Line 10 in d348fd4

#define ENCODER_PULSES_PER_REV_BITS (14)

and then not commit the code into the user's repo because this file resides in the amdc-firmware repo, which is typically a submodule of a user repo.

This workflow is of course undesired because then the change isn't tracked and the user repo no longer has all information needed to build/use the code.

Solution 1: another pitfall

The next most common approach taken is to instead redefine ENCODER_PULSES_PER_REV_BITS elsewhere in the user's repo. Depending on where this is redefined (i.e., if it overrides the #define in encoder.h before the init function in encoder.c is employed), this approach may work.

However, it is never a good practice to be redefining macros and doing so leads to confusing code. So this workflow is also undesired.

Preferred Solution

The correct way to handle this situation is to instead make use of the encoder.c driver functions to set the encoder pulses per revolution within the user's code:

AMDC-Firmware/sdk/app_cpu1/common/drv/encoder.h

Lines 15 to 16 in d348fd4

	void encoder_set_pulses_per_rev_bits(uint32_t bits);
	void encoder_set_pulses_per_rev(uint32_t pulses);

For example, introduce a new define

#define USER_ENCODER_PULSES_PER_REV_BITS (10)
#define USER_ENCODER_PULSES_PER_REV (1 << USER_ENCODER_PULSES_PER_REV_BITS)

And in the user task init:

encoder_set_pulses_per_rev_bits(USER_ENCODER_PULSES_PER_REV_BITS);

And then when reading the encoder (i.e., in the task callback function):

uint32_t position;
encoder_get_position(&position);

// Convert to radians
theta_m_enc = (double) PI2 * ( ( (double)USER_ENCODER_PULSES_PER_REV_BITS - (double)1 -(double)position + enc_theta_m_offset )/ (double) USER_ENCODER_PULSES_PER_REV);

// Mod by 2 pi
theta_m_enc = fmod(theta_m_enc,PI2);

Additional Info

• The encoder driver is specified in this AMDC docs page
• Difference between encoder_get_steps() and encoder_get_position`
  • encoder_get_steps() is used to return a count of the number of "pulses" counted by the encoder since power on. One direction of rotation causes this to increment. The other direction, decrement. This function returns a 32 bit counter that does not wrap around based on the number of bits the user specifies of the encoder.
  • encoder_get_position() is used to return the position of the shaft. The position value is determined by a counter (again counting "pulses") that wraps around based on the number of bits that the user specifies of the encoder.
    • encoder_set_pulses_per_rev_bits() and encoder_set_pulses_per_rev_bits are used by the user to specify the number of bits / pulses per revolution.
    • The position value is in units of "pulses", which is a count of the number of pulses seen by the encoder since the last revolution. This means that the user must convert it into their desired units (radians or degrees) based on the number of pulses per revolution.

Question

Since encoder_get_position() is wrapped by the number of pulses per revolution, why does our code usually still mod the converted radian value by $2\pi$?

That is, why is the last line necessary here?

// Convert to radians
theta_m_enc = (double) PI2 * ( ( (double)USER_ENCODER_PULSES_PER_REV_BITS - (double)1 -(double)position + enc_theta_m_offset )/ (double) USER_ENCODER_PULSES_PER_REV);

// Mod by 2 pi
theta_m_enc = fmod(theta_m_enc,PI2);

[noguchi-takahiro]

I suspect that this might be related to enc_theta_m_offset. For example, in the case of a 10-bit encoder, the position ranges from 0 to 1024. If the enc_theta_m_offset = 0, the range wraps from 0 to $2\pi$. However, if the enc_theta_m_offset is set to 512, for example, theta_m_enc spans from $-\pi$ to $+\pi$, which may explain why the final line is necessary? There may be other reasons as well though.

[elsevers]

Nice @noguchi-takahiro. I agree with your reasoning here.