gavv / signal-estimator

Measure characteristics of a looped back signal.
MIT License
63 stars 18 forks source link

Result ploting #54

Closed RandomJanis closed 10 months ago

RandomJanis commented 11 months ago

Implementing #47 Added measurement result curves to plot. Since signals and results have different scales, added them on seperate Y axes. It is not perfect: in losses mode there are 3 different Y dimensions, but only 2 Y axis available. Also added checkboxes to gui to show or hide curves accordingly.

gavv commented 11 months ago

Hi, thanks a lot for PR!

Just tested it, looks nice. Here are a few suggestions / questions:

  1. What do you think about moving measurements to separate plots? For example, in latency modes (latency_corr and latency_step) we can show three plots:

    • input + output
    • hw_sw latency
    • hw + hw_avg latencies

    Rationale:

    • Currently, it's hard to monitor jitter in hw and hw_sw latencies because this jitter is small compared to distance between hw and hw_sw on plot (jitter is typically withing few milliseconds, and distance can be tens of milliseconds). With separate plots each will have separate scale.

    • Currently, if hw or hw_sw happen to display in the middle of plot, it's a bit messed up with input / output.

  2. To avoid duplication, I think we could inline these checkboxes:

    image

    into legend:

    image

    e.g. we can show checkbox before each label.

    As for the grid checkbox, I think we can remove it at all and just always show grid.

  3. Since measurement reports are quite scattered, it would be useful to show points on plot, to distinguish between real data and interpolation.

    Example:

    image

  4. Maybe latency plot will look nicer if we also fill color under the line.

    Example:

    image

RandomJanis commented 10 months ago

As suggested:

As for checkboxes, with 2 curves per plot, I think they are not needed. Also changed plot background color to default, in my opinion white plot didn't fit in dark color scheme. Cursor trackers now show positions on plots. If needed I can change it back to label somewhere (I'm not sure where). image

gavv commented 10 months ago

Looks awesome, thank you so much!

Two follow-up commits:

gavv commented 10 months ago

I created a follow-up issue, partially related to this PR: #66

Also somewhat related: #67, #68 (they become relevant since we now display measurement plots).

gavv commented 10 months ago

One more follow-up issue: #69

gavv commented 10 months ago

Updated colors a bit:

image

I used colors different from input & output signals to make it clear that this plot show different data. (I used https://coolors.co)

@RandomJanis Could you please take a look whether the new colors look fine on dark theme?

gavv commented 10 months ago

One more issue: #70

RandomJanis commented 10 months ago

image Except that hardware and average hardware latency looks the same, looks ok.

gavv commented 10 months ago

Thanks, changed avg latency to red.