Skip to content

Plot decoded recorder samples in seconds#95

Open
mjc wants to merge 2 commits into
lukash:mainfrom
mjc:refloat-fix-recorder-plot
Open

Plot decoded recorder samples in seconds#95
mjc wants to merge 2 commits into
lukash:mainfrom
mjc:refloat-fix-recorder-plot

Conversation

@mjc

@mjc mjc commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

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.

Copilot AI review requested due to automatic review settings July 15, 2026 17:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 float before plotting.
  • Export the new from_float16(uint16_t) helper via conf/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.

Comment thread src/data_recorder.c Outdated
Comment thread src/conf/buffer.c
const uint16_t mantissa = x & 0x03ffu;
return sign * (exponent ? ldexpf(1024.0f + mantissa, exponent - 25)
: ldexpf(mantissa, -24));
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this code come from? I'd prefer to use the form from the stack overflow post.

@mjc mjc Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mjc mjc Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/data_recorder.c
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]));

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amend this to the first commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants