-
-
Notifications
You must be signed in to change notification settings - Fork 555
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
New subcommand plot #252
Merged
Merged
New subcommand plot #252
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The parsing is based on decode.py. The basic functionality is implemented, but see the TODO flags.
tested with output of `candump -tA` but not yet with -tz or others or without time stamps
resolved in commit 2480290
bugfix for commit 2480290
A user might want to show some values in the first subplot and all the rest in a second subplot: $ cat log | cantools plot my.dbc '*device1.current' '*device2.current' - 'current' Here the data should not be replotted. (But has been before this commit.) On the other hand a user might want to emphasize the data points in a different color to see how much time is between two messages: $ cat log | cantools plot my.dbc '*device1.current:b-' '*device1.current:ro' In this case the data should be replotted. (But was not before this commit.)
see commit 2480290
by default argparse ignores paragraph ends and lists, making especially the lists pretty unreadable. RawDescriptionHelpFormatter does not format description texts. It does not look good but is a lot better readable.
it seems cleaner to consistently call the functions of the subplot object
This has an effect on the order of the signals in the legend and will become important for testing. The order of the legend is now determined by: 1) the order of the matching regex given in the command line options 2) the name of the signal (including the message name as prefix)
I copied them from test_command_line.py but they are misleading because cantools uses format strings which require python3 (e.g. database/can/formats/arxml.py, line 25). Therefore the tests cannot pass in python2.
forgotten in commit 3fa9ef9
each test on it's own is passing but the 2nd test executed fails
bugfix for commit 3fa9ef9 problem became visible in commit b178f9b In the second test executed the system under test was using the same mock object like in the first test. But the test was comparing against the new mock object which was empty because it had not been used. Therefore the test failed. I have fixed this by mocking matplotlib globally so that all tests use the same mock object which is reset in the test setup.
instead of a list of several regex This reduces run time from 6:18 min to 5:22 min, i.e. by around 15%, for around 28.6e6 lines cantools decode output and 9 regex passed as command line arguments.
it actually ignores the information from cantools decode and only parses the information from candump which is passed through by cantools decode. Nevertheless it's important to check that the added strings don't confuse cantools plot.
idea for `plt.rcParams["date.autoformatter.minute"]` taken from: https://stackoverflow.com/a/47541788/2828064 The name of the new mock object is arbitrary, it's only purpose is to avoid that it shows up in plt.mock_calls. https://docs.python.org/3/library/unittest.mock.html#attaching-mocks-as-attributes > When you attach a mock as an attribute of another mock (or as the return value) it becomes a “child” of that mock. [...] > The exception to this is if the mock has a name. This allows you to prevent the “parenting” if for some reason you don’t want it to happen.
bug added in commit c7f5fa4. The tests passed because the changes worked as intended but matplotlib (which was replaced by a mock) is not able to handle timedelta objects. Therefore I have added a new test script which does not mock matplotlib.
commit 043fdef requires two changes to the tests: - x values are floats instead of timedelta objects (line 1229) - ignore the axes attribute of subplots which is just cosmetic and not about data (all the rest)
in commit 02b0258 I added test_plot_without_mock.py which passed when run on it's own but failed when run with `python3 -m unittest discover tests` because test_plot.py mocked matplotlib and the new test script loaded the mock instead of the real library. This is also more elegant because I am not using protected attributes anymore, compare commit 60abbfa.
belongs to commit 9918e27
The windows have a white background and so does the readme. Without the border it is not visible that toolbar and plot are in the same image.
Nice work! Please ass matplotlib as a dependency in setup.py as well. |
test 2 for commit 2572dc3
Part of release 36.1.0, available on PyPI once Github Actions finishes. |
VonSquiggles
pushed a commit
to VonSquiggles/cantools
that referenced
this pull request
May 13, 2022
* ds402: Increase delay to check status on homing start. The Statusword is examined immediately after setting the Controlword command to start homing. That is very likely to fail because of the round-trip time until the Statusword is actually updated from a TPDO. To work around that, the delay between each check of the Statusword should be moved before the actual comparison, and its default value increased. Introduce a new constant TIMEOUT_CHECK_HOMING to configure that with a default value of 100 ms. This replaces the previously used INTERVAL_CHECK_STATE which is only 10 ms by default. An even better solution would be to wait for the Statusword to be updated by a received PDO, but that would be much more complex. * Apply interval to is_homed() method as well. Same problem as in the homing() method, PDO updates of the Statusword need at least one SYNC / PDO cycle duration. * Factor out common _homing_status() method. Move the common code from is_homed() and homing() to a method for internal use. Add a comment why the delay is necessary and how it should possibly be replaced by an RPDO reception check. Should the latter be implemented, there will be only one place to change it.
VonSquiggles
pushed a commit
to VonSquiggles/cantools
that referenced
this pull request
May 13, 2022
…ols#252)" This reverts commit ebad5da. Will merge instead the PR cantools#257 with cumulative changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have added a new subcommand "plot" to visualize the received data using matplotlib, see the additions to the readme and
cantools plot --help
.I have added two test scripts for this subcommand called tests/test_plot.py and tests/test_plot_without_mock.py.
The first one contains the majority of the tests. It mocks matplotlib and checks that it's functions are called as expected.
The second one contains only a single test which checks for one example that matplotlib does not throw any exceptions and produces the expected output file.