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
db06530 to
185af2a
Compare
|
CodeRabbit caught that the recorder guarantees strictly increasing |
c6a810e to
a99ecda
Compare
a99ecda to
6420409
Compare
|
I don't know about this. First off, I used the experimental plot support before implementing the AppUI renderer. The experimental plot is buggy in itself (the transport is not resilient against comms issues and mangles the data) and I just kept the function to output to it in case it would be useful in the future (and it has rotted since then already). Not a priority spend time on for me atm. Regarding the actual encoding, not sure I understood the severity of the issue, it's not described too well. You replaced both encoder and decoder with completely different functions and the whole things looks AI-ed. Not sure you've done any testing. Note there is a decoding function in AppUI and it's also cited in the docs for the package commands (not sure those are affected too or the UB is native architecture specific). |
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.