-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Signal handles and multi-axis support #9
base: master
Are you sure you want to change the base?
Conversation
Glad to see this change was possible at all, I think the codebase can be modified fairly easy. I'm not too sure about this change, one thought I had is how far into the direction of logic analyzer / scope I would like to go. There are good tools such as sigrok which are logic analyzers which usually have this kind of view. Another idea about the multi axis would be to have an option for the plot to show a seperate axis for each signal in the plot, and allow to select it. Then the vertical zooming/panning will be done for the selected signal. I think at least all axis should be visible, preferably with the color of the signal. Maybe we also require the grid to be present multiple times? Anyway, good proposal! I'm not sure where this whole thing is going, so I would like to keep this idea around for some time. |
|
||
self._draw_curves() | ||
self._draw_legend() | ||
|
||
if self.options.show_legend: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option certainly makes sense.
@@ -77,17 +82,17 @@ def _draw_curve(self, curve): | |||
|
|||
if data: | |||
if isinstance(data[0], Aggregation): | |||
self._draw_aggregations_as_shape(data, curve_color) | |||
curve.average = self._draw_aggregations_as_shape(curve.axis, data, curve_color) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could query the average from by querying the summary of the data in view to the time series database (tsdb). This will give you a quicker result than re-calculating the average in the draw function.
Also: having the draw function doing two things (draw the plot and calculate the average) is probably not so clean, it would be better to seperate the two functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, still have to get familiar with the codebase.
@@ -109,16 +132,19 @@ def horizontal_zoom(self, amount, around): | |||
self.chart.horizontal_zoom(amount, around) | |||
# Autoscale Y for a nice effect? | |||
self.chart.autoscale_y() | |||
self.repaint() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this function do? update also triggers a paint action eventually. Does this result in snappier look / feel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handles on the left hand side otherwise lagged behind in the repainting. Not sure about the reason though. Maybe that 'update' only invalidated the chart area itself?
Yes, it is quite accessible.
The problem of plotting multiple signals with a vast different range remains. If one signal is of orders of magnitude larger than a second one, plotting them in the same area makes little sense for the secondary signal. This is typically resolved by scaling the range of either of the signals (using a dedicated axis).
Note that in the 'legend bar' branch, the selectability of signals has improved and it is always clear what signal is selected. Just to let you know... The challenge here is to keep the plot area as clean as possible. Not even all decent scopes are good at that. The visuals of the data aggregations are nice, but are at the same time already much more that a single line. Adding multiple grids and axis gives too much clutter, perhaps. |
9d0c231
to
63233b2
Compare
Maybe you can have a look at this? Not necessarily merge it right away, it probably needs some work / thinking, even if you would like this feature.
This branch prototyped multi-axis support, such that signals can individually scaled and panned, as it is quite common that correlated signals have a vast different domain. It also adds 'signal handles' to the left of the plot, similar to most digital oscilloscopes. This handle can be used for both panning and selecting that signal as being 'active'. Note that the vertical axis on the right is then changed to the active signal. While correct, this part needs some visual changes to ensure that it is clear to which signal the vertical axis belongs to.
I think something similar to this feature is useful to add the plotting of digital signals. It then allows you to organise the plot area, again similar to digital oscilloscopes.
Let me know what you think.