Rename RemoteNode.load_configuration()

[sveinse]

The RemoteNode.load_configuration() isn't named in a good way. Ref https://github.com/christiansandberg/canopen/pull/405#issuecomment-2202370883 it should be named .set_configuration() or perhaps download_configuration() to follow CANopen nomenclature.

If we rename the method, we should keep the old method name to not break the API, however with a deprecation warning, see #479

[erlend-aasland]

IMO, download_configuration() is pretty clear, as it aligns with CANopen jargon. I don't think the current name is too problematic, though, especially now that it's more clearly documented.

[acolomb]

I agree it's not a perfect name now, but frankly I never noticed the function before the recent PR and it's just a convenience function for a loop that could easily be copied to application code. I wonder who would even want to touch every possible (writable) object in a device? They should have sensible default values and a "restore defaults" function, so only setting necessary adjustments from that base is the more usual approach AFAICT.

I'd say let's not open any cans of worms regarding deprecation and renaming over this. With a better docstring explaining what it does, the API can stay as it is.

[erlend-aasland]

FTR, load_configuration() is used in examples/simple_ds402_node.py, so it may be commonly used in code bases using canopen.

[sveinse]

IMO, download_configuration() is pretty clear, as it aligns with CANopen jargon. I don't think the current name is too problematic, though, especially now that it's more clearly documented.

Since the newly updated docs use the CANopen-esque "download" word, this makes sense.

[acolomb]

FTR, load_configuration() is used in examples/simple_ds402_node.py, so it may be commonly used in code bases using canopen.

That might not be the best coding style for real applications though, but more to serve as an example. But applications and network requirements can vary widely, so if the API user wants to flood the bus with SDOs in a script, then we have a convenience flooder method for sure.

I lean toward lean, i.e. configuring devices once during commissioning, then saving, and not worry about all parameters during each startup of the whole machine. That's probably why I never even looked for such a "download everything" method.

[sveinse]

I lean toward lean, i.e. configuring devices once during commissioning, then saving, and not worry about all parameters during each startup of the whole machine. That's probably why I never even looked for such a "download everything" method.

What the author of PR #405 wants is not "download everything" to the node, but a way to configure/setup the PDO object from the loaded OD. So we're back at not really knowing what the actual use cases are for .load_configuration().

[sveinse]

The examples/simple_ds402_node.py example is IMHO a bit back-and-forth and highly dependent on what the remote node supports:

# L52:  This _downloads_ the PDO configuration to the remote node and assumes
# a device which have the ability to set the PDO configuration
node.load_configuration()

# L69:  This _uploads_ the PDO configuration from the remote node
node.tpdo.read()

# L82 This _uploads_ the PDO configuration from the remote node
node.rpdo.read()

If the remote PDO configuration in the remote node is blank at start-up the load_configuration() step is needed here. If its not needed, the two read() calls will sync (upload) the remote node with the local PDO configuration and load_configuration() can be omitted.

If the remote node has a static PDO configuration and with a complete OD that describes the PDO, everything may be done without any SDO IO:

node = RemoteNode(..., object_dictionary=something)
node.tpdo.read(from_od=True)
node.rpd.read(from_od=True)

edit: Perhaps this is a separate issue about the example and not the rename, thou :P

[erlend-aasland]

Yes, sorry for distracting you with the example. But that borderlines a discussion I just started: I'm suggesting to use Diátaxis as a north star for the canopen docs :)

[acolomb]

What the author of PR #405 wants is not "download everything" to the node, but a way to configure/setup the PDO object from the loaded OD.

Yes, but we agreed that it was simply the wrong method. For that use, node.pdo.read(from_od=True) is the right call.

Your assessment of the example code is correct. The only plausible reason for doing pdo.read(from_od=False) after SDO-based changes to the device state would be if the node accepts the mappings in slightly modified form, but doesn't complain about it via SDO abort. Which is not unrealistic among real devices out in the field.

Normally, changes to the PDO config should go through the library's PDO methods, including pdo.write() if not saved in the device. Note that having a shared ObjectDictionary instance for several (remote) nodes is a well-supported use case, thus the PDO configuration is usually not mirrored back to that local OD object. It is a blueprint for a device after all, not a synchronized mirror of the device's actual OD. The important part is matching the node.pdo... objects against the real device OD. Downloading settings from the memory-based Python canopen OD instance is mostly just for supporting DCF files.

[acolomb]

We probably overlooked #427, where applying the PDO related values from a DCF is actually made functional in the first place. It uses the PDO classes to apply the provided values and skips the usual SDO transfer. And it fixes PdoBase.read(from_od=True) to actually honor the provided DCF values instead of the EDS default values.

But we will need to adjust the example accordingly, which is confusing / inefficient as @sveinse has pointed out.

EDIT: Created #496 to track the issues regarding the example code.

[sveinse]

Since we're spawning of many sub-discussions from this: I read we have consensus that we don't really see the need to reaname the .load_configuration() method? If no objections to this, I'll close this issue.