Plot decoded recorder samples in seconds#95
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the experiment plotting path for the data recorder so that recorded samples are decoded from the stored 16-bit float format before plotting, and recorded tick timestamps are converted to seconds for the plot x-axis. This aligns the plotted output with the recorder’s on-device storage format while keeping the recorded/binary storage format unchanged.
Changes:
- Convert recorded timestamps from system ticks to seconds when sending plot points.
- Decode stored half-precision (“float16”) sample values back to
floatbefore plotting. - Export the new
from_float16(uint16_t)helper viaconf/buffer.h.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/data_recorder.c |
Decode stored sample values and convert tick timestamps to seconds when plotting. |
src/conf/buffer.h |
Exposes from_float16 for use outside buffer.c. |
src/conf/buffer.c |
Implements from_float16 conversion helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const uint16_t mantissa = x & 0x03ffu; | ||
| return sign * (exponent ? ldexpf(1024.0f + mantissa, exponent - 25) | ||
| : ldexpf(mantissa, -24)); | ||
| } |
There was a problem hiding this comment.
Where does this code come from? I'd prefer to use the form from the stack overflow post.
There was a problem hiding this comment.
I missed that the existing encoder’s cited Stack Overflow answer also included the matching decoder.
This seems to be a small piece of "works on my compiler" archaeology, in the original Stack Overflow answer.
The encoder from the post evaluates an invalid shift when the exponent field is at most 93 or at least 126, including both 0.0f and ordinary values such as 1.0f. The decoder does so whenever the mantissa is zero, including zero and normalized powers of two. Its sign-bit shift also needs to be made unsigned. UBSan catches these, though the current Cortex-M4 output happens to produce the intended results.
There was a problem hiding this comment.
Here’s how to do both, without UB, while retaining branchless output with GCC 13.3.1 and the current Cortex-M4 -Os flags:
#include <string.h>
uint16_t to_float16(float x) {
uint32_t bits;
memcpy(&bits, &x, sizeof(bits));
const uint32_t b = bits + 0x00001000;
const uint32_t e = (b & 0x7F800000) >> 23;
const uint32_t m = b & 0x007FFFFF;
/*
* C's conditional operator only evaluates the selected side, so
* (125 - e) is used as a shift count only for 102 <= e <= 112.
*/
const uint32_t denormalized = (e < 113 && e > 101)
? ((((0x007FF000 + m) >> (125 - e)) + 1) >> 1)
: 0;
return (b & 0x80000000) >> 16 |
(e > 112) *
((((e - 112) << 10) & 0x7C00) | (m >> 13)) |
denormalized |
(e > 143) * 0x7FFF;
}
float from_float16(uint16_t x) {
const uint32_t e = (x & 0x7C00) >> 10;
const uint32_t m = (x & 0x03FF) << 13;
const float mf = (float)m;
uint32_t mf_bits;
memcpy(&mf_bits, &mf, sizeof(mf_bits));
const uint32_t v = mf_bits >> 23;
/*
* For m == 0, this produces a safe shift count of zero.
* For m != 0, 150 - v is already in the valid 1..10 range.
*/
const uint32_t shift = (150 - v) * (m != 0);
const uint32_t bits =
((uint32_t)(x & 0x8000) << 16) |
(e != 0) * ((e + 112) << 23 | m) |
((e == 0) & (m != 0)) *
((v - 37) << 23 |
((m << shift) & 0x007FE000));
float result;
memcpy(&result, &bits, sizeof(result));
return result;
}to_float16 retains the same 104-byte code size, with a different instruction sequence.
from_float16 pays an extra 12 bytes for the safe zero selection. no data-dependent branches in either.
The memcpy calls are the standard defined bit-cast idiom and optimize away with both GCC and Clang.
Clang lowers this conditional to branches. Masking the shift count with & 31 keeps it branchless there too, but grows Clang’s encoder from 104 to 106 bytes and GCC’s from 104 to 108 bytes, so I’ve left the GCC-optimal form here.
original encoder asm: shift unconditionally, discard invalid results after.
revised encoder asm: predicate shift with an IT block.
decoder does moveq to replace the shift count with zero when the mantissa is zero.
I understand if you might not want this, hence posting it as a comment
| VESC_IF->plot_send_points( | ||
| sample->time * (1.0f / SYSTEM_TICK_RATE_HZ), from_float16(sample->values[i]) | ||
| ); | ||
| VESC_IF->plot_send_points(time, from_float16(sample->values[i])); |
Decode stored half-precision recorder values before plotting and convert tick timestamps to seconds on the x-axis.
Recorder storage and binary data responses are unchanged.