pdo: Skip saving if no COB-ID was set (fixes #445)

[friederschueler]

fixes #445

• changed save() Method of PdoBase to be more robust
• added some type hints
• removed unused variables
• added Test for PDO saving
• fixed long line (comments do count!)

[friederschueler]

Maybe the log output is overkill, also if you don't like the other minor changes in the file I can exclude them.

[acolomb]

Thanks for your contributions. Indeed I would prefer separate PRs for features and other drive-by cleanups. Nevertheless, see inline comments on the individual changes.

[acolomb]

This type is not imported, and probably can't be to avoid a cycle. How does this usually work with type hints to higher-level classes?

[sveinse]

One can avoid circular import by adding conditional imports:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from canopen.network import Network

And I do recommend using

from __future__ import annotations

# This allows
network: Optional[Network] = None

# Instead of
network: Optional["Network"] = None

[friederschueler]

I am aware of those things, I used the string-notation because it allows to type hint without actually importing the classes, at least in VSCode and PyCharm it autocompletes with string-hints.
Of course this should be consistent through the project.
What is the minimal python version supported? 3.7? Then I would prefer the TYPE_CHECKING if-clause to prevent circular imports. Another solution is local imports, but that is ugly too imho.

[sveinse]

With PR #428 the minimum Python version will be 3.8

I believe its not advisable to rely on typing parsing without importing it, even if some IDEs have extra logic that searches the project. E.g. a github action that runs mypy is not unrealistic.

[friederschueler]

I changed pdo/base.py to use correct type hints now and Iused the if-clause as workaround for cyclic imports.

[acolomb]

After reading up on Postponed Evaluation of Annotations (PEP-563 and PEP-649), I agree that both the if TYPE_CHECKING and the __future__.annotations import are the best way to approach this since we are probably targeting Python 3.8 for the next release. Therefore I opened #451 to start collecting these changes. The relevant part here should be factored out and moved to that PR instead.

[friederschueler]

Ok, I hope reduced my PR to a minimal but usable state.

[acolomb]

Thanks for keeping up with the reviews and comments. Some remaining styling nits suggested.

Just to be clear, on a high-level view: Previously saving a PDO with cob_id=None failed with an exception. Now it is simply skipped with a debug message. This doesn't change the ability to disable PDOs though?

What I mean is simply loading a node's EDS file and wanting to set all PDOs to disabled. Without first reading the configuration from it. So you'll need to set .cob_id = 0 and .enabled = False before running .save(). Just as before, but with a different (silent) outcome if you forget to clear the COB ID.

I think we should mention that in the NEWS for the next release. But it's not really a breaking change, just a less fatal failure mode for broken code. Still wondering whether a warning would be more appropriate, since trying to save without a COB ID (still None) really is an API usage error. What do you think?

[acolomb]

I also don't like silent fails, but I even more dislike broken default code. My Use-Case is the following: I have a lot of PDOs on devices, I have real device (aka master) on the bus, but for testing I fake some bus communication with my python canopen. So all my PDOs are already setup and configured by the real master. I want to use those PDOs without reading them all over the real bus, this triggers errors because the bus load is pretty high. Now I want to change some PDOs or verify (read) some PDOs without iterating over all devices and all PDOs to set the cob-id to 0x0.

That use case is perfectly valid and I can add another example I've encountered: The master is implemented in python-canopen and configures the other nodes' communication. But when it restarts for whatever reason (especially during development), there is no need to switch all other nodes back to PRE-OP, exchange lots of SDOs and go back to OPERATIONAL, just to reach the same state again. The nodes may even have their configuration saved in non-volatile memory and thus never need PDO configuration. For a fast startup time, the master can then be coded to simply assume a fixed PDO layout, without reading via SDO first.

I don't care that much about what the original intention was to allow None as COB ID value. Just that we don't take away possible uses of our API for no good reason. I wanted to confirm that disabling PDOs using save() is still possible with this change. Which I think we both agree on.

The question remains: Is a log warning appropriate to get the API user's attention and let them initialize cob_id to zero for unwanted PDOs? Or is that overkill and there are many valid cases where PDO descriptions are not initialized properly before saving, so that a warning will be too noisy?

[friederschueler]

I don't care that much about what the original intention was to allow None as COB ID value. Just that we don't take away possible uses of our API for no good reason. I wanted to confirm that disabling PDOs using save() is still possible with this change. Which I think we both agree on.

Yes we do. If we remove usage options, that would be a bad bugfix.

The question remains: Is a log warning appropriate to get the API user's attention and let them initialize cob_id to zero for unwanted PDOs? Or is that overkill and there are many valid cases where PDO descriptions are not initialized properly before saving, so that a warning will be too noisy?

The more I think about it, I think we should just give them a "warning" with logger.info() as all the other messages in this method also use this level. In my use-case it is encouraged to only save the needed pdos, not everything. I also think we should add another line if we enable the PDO, because we also log that we temporarily disable the PDO. I know there is also a logger line when subscribing, but you can do that without saving first:

    if self.enabled:
        cob_id = self.cob_id | (RTR_NOT_ALLOWED if not self.rtr_allowed else 0x0)
        self.com_record[1].raw = cob_id
        logger.info("Setting COB-ID 0x%X and re-enabling PDO", cob_id)
        self.subscribe()

[acolomb]

I guess Info level works fine. We can still fine-tune that in a later release if people have issues with that.

The extra log message seems good, although I would prefer it before the actual SDO access.

[friederschueler]

done

[acolomb]

Looks good, thanks for keeping up with the requested changes.