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

Rename RemoteNode.load_configuration() #480

Closed
sveinse opened this issue Jul 2, 2024 · 11 comments
Closed

Rename RemoteNode.load_configuration() #480

sveinse opened this issue Jul 2, 2024 · 11 comments

Comments

@sveinse
Copy link
Collaborator

sveinse commented Jul 2, 2024

The RemoteNode.load_configuration() isn't named in a good way. Ref #405 (comment) 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
Copy link
Contributor

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
Copy link
Collaborator

acolomb commented Jul 2, 2024

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
Copy link
Contributor

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

@sveinse
Copy link
Collaborator Author

sveinse commented Jul 2, 2024

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
Copy link
Collaborator

acolomb commented Jul 2, 2024

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
Copy link
Collaborator Author

sveinse commented Jul 2, 2024

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
Copy link
Collaborator Author

sveinse commented Jul 2, 2024

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
Copy link
Contributor

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
Copy link
Collaborator

acolomb commented Jul 3, 2024

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
Copy link
Collaborator

acolomb commented Jul 4, 2024

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
Copy link
Collaborator Author

sveinse commented Jul 6, 2024

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.

@acolomb acolomb closed this as not planned Won't fix, can't repro, duplicate, stale Jul 8, 2024
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

No branches or pull requests

3 participants