[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

lack of matplolib does not error until actually running subcommand #265

Merged
merged 4 commits into from
Feb 9, 2021

Conversation

altendky
Copy link
Contributor
@altendky altendky commented Feb 3, 2021

Follow up for #259.

@coveralls
Copy link
coveralls commented Feb 3, 2021

Coverage Status

Coverage decreased (-4.4%) to 92.68% when pulling f35ba93 on altendky:patch-1 into 52b1421 on eerimoq:master.

@altendky
Copy link
Contributor Author
altendky commented Feb 9, 2021

I don't presently see any test environments without matplotlib nor any subprocessed tests. Will those need to be added or can there be an exception here for the slightly reduced coverage?

$ venv/bin/cantools plot x y
error: The matplotlib package not installed and is required for producing plots.
$ venv/bin/pip install matplotlib
Collecting matplotlib
  Using cached matplotlib-3.3.4-cp39-cp39-manylinux1_x86_64.whl (11.5 MB)
Requirement already satisfied: pillow>=6.2.0 in ./venv/lib/python3.9/site-packages (from matplotlib) (8.1.0)
Requirement already satisfied: python-dateutil>=2.1 in ./venv/lib/python3.9/site-packages (from matplotlib) (2.8.1)
Requirement already satisfied: numpy>=1.15 in ./venv/lib/python3.9/site-packages (from matplotlib) (1.20.1)
Requirement already satisfied: kiwisolver>=1.0.1 in ./venv/lib/python3.9/site-packages (from matplotlib) (1.3.1)
Requirement already satisfied: pyparsing!=2.0.4,!=2.1.2,!=2.1.6,>=2.0.3 in ./venv/lib/python3.9/site-packages (from matplotlib) (2.4.7)
Requirement already satisfied: cycler>=0.10 in ./venv/lib/python3.9/site-packages (from matplotlib) (0.10.0)
Requirement already satisfied: six in ./venv/lib/python3.9/site-packages (from cycler>=0.10->matplotlib) (1.15.0)
Installing collected packages: matplotlib
Successfully installed matplotlib-3.3.4
$ venv/bin/cantools plot x y
error: [Errno 2] No such file or directory: 'x'

@eerimoq
Copy link
Collaborator
eerimoq commented Feb 9, 2021

It would be nice with a test if it's not too much work to write it. It's probably easiest to create a file called matplotlib.py and change PYTHONPATH to find it before the installed matplotlib package. matplotlib.py should contain raise ImportError().

@altendky
Copy link
Contributor Author
altendky commented Feb 9, 2021

No, I'm not going to hack around with PYTHONPATH etc etc.

cantools/subparsers/plot.py Outdated Show resolved Hide resolved
cantools/subparsers/plot.py Outdated Show resolved Hide resolved
cantools/subparsers/plot.py Outdated Show resolved Hide resolved
cantools/subparsers/plot.py Outdated Show resolved Hide resolved
@eerimoq eerimoq merged commit d396fbd into cantools:master Feb 9, 2021
AxelVoitier added a commit to AxelVoitier/cantools that referenced this pull request Feb 17, 2021
Import-catch philosophy similar to the one used in cantools#265 for matplotlib.
@altendky altendky deleted the patch-1 branch February 17, 2021 13:49
VonSquiggles pushed a commit to VonSquiggles/cantools that referenced this pull request May 13, 2022
* ds402: Skip explicit change to SWITCHED ON state.

The BaseNode402.homing() method tries to enter state SWITCHED ON upon
entry.  That's unnecessary, the application should handle these
transitions.  But more importantly, it actually fails in many cases,
namely if the previous state is SWITCH ON DISABLED, the default
power-up state of most devices.  There is an automatic way to reach
OPERATION ENABLED over multiple intermediate steps, but the library
does not know how to reach SWITCHED ON from any other state than
OPERATION ENABLED or READY TO SWITCH ON.  In addition, setting the
op_mode property will already change to SWITCHED ON only if coming
from OPERATION ENABLED (which is usually a good idea to avoid
unexpected movement).

Note that switching the operation mode to HOMING is actually safe in
any power state.

* p402: Make HOMING_TIMEOUT_DEFAULT configurable per instance.

When the HOMING_TIMEOUT_DEFAULT class attribute is overridden as an
object attribute, the argument default value definition in homing()
still uses the old value from the class attribute.  Check the argument
and apply the default value at runtime to pick up the updated default
timeout if the argument is omitted.

* 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.

* p402: Wait for TPDO instead of timed statusword checking.

Several methods loop around waiting for some expected statusword
value, usually calling time.sleep() with the INTERVAL_CHECK_STATE or
INTERVAL_CHECK_HOMING constants.  Replace that with a call to
check_statusword(), which will only block until the TPDO is received.

This allows for possibly shorter delays, because the delay can be cut
short when the TPDO arrives in between.  But rate limiting the
checking loops is still achieved through the blocking behavior of
check_statusword().

If no TPDO is configured, the statusword will fall back to checking
via SDO, which will also result in periodic checks, but only rate
limited by the SDO round-trip time.  Anyway this is not a recommended
mode of operation, as the warning in _check_statusword_configured()
points out.

* p402: Use RPDO to set the operation mode if possible.

Fall back to the previous behavior using SDO if the relevant object
0x6060 is not mapped to an RPDO.

* p402: Check PDO configuration for the Operation Mode objects.

Switching operation modes for several drives simultaneously must be
done via PDO.  There is still a fallback mechanism via SDO, but a
warning should be issued when that is about to be used.
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