samuelsadok
commented
3 months ago 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}}$
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_measleave some of it uninitializedOn 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:
https://github.com/odriverobotics/ODrive/blob/6c3cefdeacce600ba4524f4adc5fd634c7bebd18/Firmware/MotorControl/sensorless_estimator.cpp#L39-L44
Because as far as I can tell, the struct used to store the current measurement has 3 fields, not 2:
https://github.com/odriverobotics/ODrive/blob/6c3cefdeacce600ba4524f4adc5fd634c7bebd18/Firmware/odrive-interface.yaml#L12
And this means that in the best case, on line 43 we're only setting two of them:
https://github.com/odriverobotics/ODrive/blob/6c3cefdeacce600ba4524f4adc5fd634c7bebd18/Firmware/MotorControl/sensorless_estimator.cpp#L43
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_measto 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:
https://github.com/odriverobotics/ODrive/blob/6c3cefdeacce600ba4524f4adc5fd634c7bebd18/Firmware/MotorControl/sensorless_estimator.cpp#L58-L69
This works fine for now, but this is the only instance where Eta is written to before it is read in line 73:
https://github.com/odriverobotics/ODrive/blob/6c3cefdeacce600ba4524f4adc5fd634c7bebd18/Firmware/MotorControl/sensorless_estimator.cpp#L73
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:
https://github.com/odriverobotics/ODrive/blob/6c3cefdeacce600ba4524f4adc5fd634c7bebd18/Firmware/MotorControl/sensorless_estimator.cpp#L52-L55
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.