[go: nahoru, domu]

Skip to content
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 67 commits into from
Jan 4, 2021
Merged

New subcommand plot #252

merged 67 commits into from
Jan 4, 2021

Conversation

erzoe
Copy link
Contributor
@erzoe erzoe commented Jan 4, 2021

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.

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
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.)
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.
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.
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.
@coveralls
Copy link
coveralls commented Jan 4, 2021

Coverage Status

Coverage increased (+0.1%) to 97.19% when pulling 0b87191 on erzoe:master into 7d97b43 on eerimoq:master.

@eerimoq
Copy link
Collaborator
eerimoq commented Jan 4, 2021

Nice work!

Please ass matplotlib as a dependency in setup.py as well.

@eerimoq eerimoq merged commit 6ab0eac into cantools:master Jan 4, 2021
@eerimoq
Copy link
Collaborator
eerimoq commented Jan 4, 2021

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants