Two instances of UB and a couple of questions about implementation

[felix91gr]

Hi yall, I'm working on porting https://github.com/odriverobotics/ODrive/blob/master/Firmware/MotorControl/sensorless_estimator.cpp to a proof of concept project of a friend of mine.

I have 2 instances of UB to report in that implementation, and something to ask about the alpha-beta (Clarke's) transform's implementation that I haven't been able to fully understand.

First, the UB.

Undefined Behavior

Default values for current_meas leave some of it uninitialized

On the section between lines 39 and 44, we check if the motor is armed and if it isn't, we set the current measurement values to 0. Or that's the intent, anyway:

ODrive/Firmware/MotorControl/sensorless_estimator.cpp

Lines 39 to 44 in 6c3cefd

	auto current_meas = axis_->motor_.current_meas_;
	if (!axis_->motor_.is_armed_) {
	// While the motor is disarmed the current is not measurable so we
	// assume that it's zero.
	current_meas = {0.0f, 0.0f};
	}

Because as far as I can tell, the struct used to store the current measurement has 3 fields, not 2:

ODrive/Firmware/odrive-interface.yaml

Line 12 in 6c3cefd

struct Iph_ABC_t { float phA; float phB; float phC; };

And this means that in the best case, on line 43 we're only setting two of them:

ODrive/Firmware/MotorControl/sensorless_estimator.cpp

Line 43 in 6c3cefd

current_meas = {0.0f, 0.0f};

Working with uninitialized memory is Undefined Behavior, and even if it works currently, any future or alternative version of the compiler is allowed to break it.

As mitigation against this undefined behavior, I'd suggest current_meas to be initialized to {0.0f, 0.0f, 0.0f} in line 43 instead.

Eta (η) estimator is not initialized on creation, which makes it fragile against future changes

The vector function eta (η) is not initialized on creation. It is initialized afterwards, in the loop between lines 59 and 69:

ODrive/Firmware/MotorControl/sensorless_estimator.cpp

Lines 58 to 69 in 6c3cefd

	float eta[2];
	for (int i = 0; i <= 1; ++i) {
	// y is the total flux-driving voltage (see paper eqn 4)
	float y = -axis_->motor_.config_.phase_resistance * I_alpha_beta[i] + V_alpha_beta_memory_[i];
	// flux dynamics (prediction)
	float x_dot = y;
	// integrate prediction to current timestep
	flux_state_[i] += x_dot * current_meas_period;
	// eta is the estimated permanent magnet flux (see paper eqn 6)
	eta[i] = flux_state_[i] - axis_->motor_.config_.phase_inductance * I_alpha_beta[i];
	}

This works fine for now, but this is the only instance where Eta is written to before it is read in line 73:

ODrive/Firmware/MotorControl/sensorless_estimator.cpp

Line 73 in 6c3cefd

float est_pm_flux_sqr = eta[0] * eta[0] + eta[1] * eta[1];

This means that Eta's initialization depends on the loop that comes beforehand. Any changes that make reading Eta possible earlier could make it so the driver reads an uninitialized value. Doing so would not only make the values unpredictable; Eta is a cosine-sine pair, which means it has a very well-defined domain in which it is valid. The rest of the program assumes (of course; why wouldn't it?) that Eta is a valid element of its domain. Reading an uninitialized Eta would throw all sorts of invariants away, and would make the driver's behavior completely unpredictable.

As mitigation against this undefined behavior, I'd suggest Eta to be initialized to some value that is valid inside its domain. A [1, 0] value would suffice, or anything else that is a valid sin-cos pair.

The alpha-beta (Clarke's) transform implementation

Between the lines 52 and 55, we transform the current measurements using the alpha-beta transform, in order to get a value inside the fixed alpha-beta-zero reference frame:

ODrive/Firmware/MotorControl/sensorless_estimator.cpp

Lines 52 to 55 in 6c3cefd

	// Clarke transform
	float I_alpha_beta[2] = {
	current_meas->phA,
	one_by_sqrt3 * (current_meas->phB - current_meas->phC)};

This version of the transform however, seems slightly strange to me. In the wikipedia section for the Simplified Alpha-Beta Transformation, which is the version of the Clarke Transform that leaves us in the alpha-beta-zero reference frame, it says that the simplified transformation matrix looks like this:

The current implementation however, does none of the above. Instead, it uses a matrix more like this one:

I think the math checks out, but it still surprised me for a day or two. ¿Is this the intended matrix? It looks like it was optimized to be this way. I'm just asking to be completely sure, since automatic control math is kind of at the border of my expertise.

[samuelsadok]

Default values for current_meas

Yes that's true. In current-gen ODrive firmware (see note) this part is slightly refactored, so it's not an issue anymore, but you might wanna change it in your port or feel free make a PR with this change.

Eta is not initialized

The for loop can be seen as the initialization of eta. If we wanna be pedantic, we could put the content of the for-loop into a function and then have the declaration and initialization on the same line, but that would reduce readability in other ways. A default init value would be misleading IMO cause it's never used and distracts from the fact that the for-loop does the initialization.

Clarke's transform

Yup the transforms on Wikipedia and in ODrive firmware are the same:

• First row of the matrix: (using second form from your Wikipedia screenshot):
  $i_\alpha = [1\ 0]\ [i_a\ i_b]^T = [1\ 0\ 0]\ [i_a\ i_b\ i_c]^T$
• Second row (using first form from your Wikipedia screenshot):
  $i_\beta=\frac{2}{3} [0\ \frac{\sqrt{3}}{2}\ -\frac{\sqrt{3}}{2}]\ [i_a\ i_b\ i_c]^T=[0\ \frac{\sqrt{3}}{3}\ -\frac{\sqrt{3}}{3}]\ [i_a\ i_b\ i_c]^T=[0\ \frac{1}{\sqrt{3}}\ -\frac{1}{\sqrt{3}}]\ [i_a\ i_b\ i_c]^T$
  because $\frac{\sqrt{3}}{3}=\frac{\sqrt{3}}{3}\cdot\frac{\sqrt{3}}{\sqrt{3}}=\frac{3}{3\sqrt{3}}=\frac{1}{\sqrt{3}}$