RT Capabilities Part 1 - robot-log-visualizer #80
Conversation
There was a problem hiding this comment.
Hi @nicktrem,
First of all, thank you for working on this feature. It will be really useful to have. Still, there are some things that should be fixed before merging the PR. In general:
- I would keep the same style as the repo, so use snake_case for functions and variables and CamelCase for classes.
- I would keep yarp and blf as optional dependencies. Indeed, the visualizer is installed via pip, and blf is not included.
- The real-time logging can be moved to a separate class. Indeed, it's just a different provider compared to the mat file. Then the GUI may be attached to that real-time provider.
- I would avoid replotting the signal from scratch (i.e., delete the axes). In theory, you can use the animation and just modify the content of the variables of the matplotlib line.
- You should avoid to use prints, In case of warning/error you can use the logger provided by the gui
| if not self.realtimeNetworkInit: | ||
| yarp.Network.init() | ||
|
|
||
| param_handler = blf.parameters_handler.YarpParametersHandler() |
There was a problem hiding this comment.
I'm fine to have config files. However, I would keep blf deps relegated only for the real-time logging and use qt config
| # Update the indexes of plotData before deletion | ||
| for i in range(index, len(self.plotData.keys()) - 1): | ||
| self.plotData[i] = self.plotData[i + 1] | ||
| # Remove the last key | ||
| del self.plotData[list(self.plotData.keys())[-1]] |
There was a problem hiding this comment.
can you explain why this is required? We can discuss this F2F
There was a problem hiding this comment.
@nicktrem can you check this comment?
Co-authored-by: Giulio Romualdi <[email protected]>
|
As discussed today f2f with @GiulioRomualdi and @traversaro the remaining parts of this PR to be addressed are:
These points have all been addressed today. @GiulioRomualdi if you have any further questions feel free to ask me 😄 |
| @@ -11,6 +11,10 @@ | |||
| import idyntree.swig as idyn | |||
|
|
|||
|
|
|||
| # for real-time logging | |||
| import yarp | |||
There was a problem hiding this comment.
@GiulioRomualdi should we lazy load also yarp or we just need to lazy load blf?
fyi @nicktrem
| 0, self.signal_provider.end_time - self.signal_provider.initial_time | ||
| ) | ||
| if realtime_plot: | ||
| #self.axes.autoscale() |
There was a problem hiding this comment.
If this is not needed we can remove it.
| @@ -239,15 +239,25 @@ def setupUi(self, MainWindow): | |||
| icon = QtGui.QIcon.fromTheme("exit") | |||
| self.actionQuit.setIcon(icon) | |||
| self.actionQuit.setObjectName("actionQuit") | |||
|
|
|||
| # Add the GUI components for the open action | |||
There was a problem hiding this comment.
This is an autogenerated file and cannot be touched the correct approach here is to modify the UI file directly with qtcreator
| # Disable these buttons for RT communication | ||
| self.ui.startButton.setEnabled(False) | ||
| self.ui.timeSlider.setEnabled(False) | ||
| self.network_thread = threading.Thread(target=self.establish_connection, args=(root,)) |
There was a problem hiding this comment.
can you use qtthread here?
|
I'm cleaning a bit the PR to move the blf deps optional so the feature can be ported to pip |
|
Hi @nicktrem let me know when it's ready so I can start checking it :) |
|
@nicktrem @GiulioRomualdi there is any update on this? Thanks! |
|
It is good to go on my end @GiulioRomualdi let me know if you have any questions |
|
According to @GiulioRomualdi he did some tests on the real robot, and he did not remember well, but when using the RT logger, something related to saving the data was not working. |
|
That issue is on the BLF side, not the robot-log-visualizer side. I can investigate that separately, however, that issue is separate from this PR |
This PR for the robot-log-visualizer is a smaller simpler version of the original PR for the robot-log-visualizer and just contains the bare-bone code needed to visualize data online. The point of breaking up the original PR was to simplify the merge process for the robot-log-visualizer.