Skip to content

Conversation

@vongostev
Copy link
Contributor

In the library we have some methods to load settings from OD, for example node.rpdo.read(from_od=True), but we can not do this for entire configuration.
This PR fixes it.

The fixed bug relates to the inability to use a custom can.BusABC implementation in the Network connection process due to hard-coded instantiation of the can.Bus object in the `connect` method.
In the library we have some methods to load settings from OD, for example `node.rpdo.read(from_od=True)`, but we can not do this for entire configuration.
This PR fixes it.
@acolomb
Copy link
Member

acolomb commented Apr 24, 2024

What exactly is the use case for this? load_configuration() actually does not upload the objects from the remote node, but it downloads the OD variable values to the remote node. Then it goes on to read back the applied PDO configuration.

How does it make sense to buy in to the vast exchange of SDO downloads, but then saving a couple of SDO uploads for reading back the actually accepted configuration?

@sveinse
Copy link
Collaborator

sveinse commented May 15, 2024

Perhaps its better to just call self.pdo.read(True) and skip the from_od argument to the function?

@acolomb
Copy link
Member

acolomb commented Jun 29, 2024

Perhaps its better to just call self.pdo.read(True) and skip the from_od argument to the function?

The .load_configuration() method is more equivalent to pdo.write(), though. It goes through all objects in the OD actually and downloads the values to the remote node via SDO. Then to avoid having mismatching information about the PDO-related objects, it reads back (uploads) the PDO records again via SDO.

In essence, there is nothing to fix here IMHO, just don't call node.load_configuration() if you want to work with the values from the OD. They are automatically read in when initializing the node's OD from a DCF file (or defaults from the EDS). The only missing step would be to update the PDO configuration as well, which node.pdo.read(from_od=True) does.

@sveinse
Copy link
Collaborator

sveinse commented Jul 2, 2024

The PDO state is independent of the OD state and when the RemoteNode() class instance is created. The PDO objects are created and referenced to the OD, but their mapping state vars, like .cob_id, .rtr_allowed etc. are only populated with a call to PdoMap.read(). So the PDO objects are not useable out of box. RemoteNode(..., load_od=True) can be used but it ends up doing remote SDO-requests since PdoMap.read() has default from_od=False.

Maybe PdoMaps().__init__() when the PdoMap() instance is created from the OD should also call PdoMap.read(from_od=True) to sync the PDO state with the (default) OD state? This way the PDOs would ready to use after creating the RemoteNode instance. If the user needs to update from the remote PDO configuration, the PdoMap.read(from_od=False) will do the job as per usual.

@erlend-aasland
Copy link
Contributor

André:

The .load_configuration() method is more equivalent to pdo.write(), though. It goes through all objects in the OD actually and downloads the values to the remote node via SDO. Then to avoid having mismatching information about the PDO-related objects, it reads back (uploads) the PDO records again via SDO.

It would be nice to add hint about this in the .load_configuration docs.

@erlend-aasland
Copy link
Contributor

Maybe PdoMaps().__init__() when the PdoMap() instance is created from the OD should also call PdoMap.read(from_od=True) to sync the PDO state with the (default) OD state? This way the PDOs would ready to use after creating the RemoteNode instance. If the user needs to update from the remote PDO configuration, the PdoMap.read(from_od=False) will do the job as per usual.

First: I think this discussion belongs on the tracker, not on a PR.
Second: I prefer APIs without side-effects, especially when instantiating objects. IOW, when I create a RemoteNode instance, I would not want it to default to doing I/O-operations; I prefer to control those explicitly, since it makes for code that is easier to reason about.

@sveinse
Copy link
Collaborator

sveinse commented Jul 2, 2024

Second: I prefer APIs without side-effects, especially when instantiating objects. IOW, when I create a RemoteNode instance, I would not want it to default to doing I/O-operations; I prefer to control those explicitly, since it makes for code that is easier to reason about.

It doesn't do IO unless RemoteNode(..., load_od=True) is used.

@sveinse
Copy link
Collaborator

sveinse commented Jul 2, 2024

One cannot use PDOs without some initialization when a RemoteNode() is created, and that's what is lacking. I agree with @acolomb that I'm not so sure that it belongs in .load_configuration() thou.

First: I think this discussion belongs on the tracker, not on a PR. Second

Since this discussion is bigger and more principal than the PR, perhaps we should move the discussion to an issue?

@acolomb
Copy link
Member

acolomb commented Jul 2, 2024

The discussion here is mainly relevant to the PR, as I think it fixes something which isn't broken and should thus not be merged.

Second: I prefer APIs without side-effects, especially when instantiating objects

That's a fair point and as things stand, doing the I/O operations during object creation is opt-in. Frankly I didn't even know about that possible argument, and would never use it myself.

Maybe PdoMaps().__init__() when the PdoMap() instance is created from the OD should also call PdoMap.read(from_od=True) to sync the PDO state with the (default) OD state?

That's a good idea. It doesn't cost much to transfer the (offline) OD info to the PDO states, and it saves doing a node.pdo.read(from_od=True) call. In case of a DCF file, no further SDO exchanges would be necessary if it can be assumed that the DCF correctly mirrors the actual device OD.

So back to the actual PR, I see we have consensus that .load_configuration() is not the right place to add this behavior flag (as it really is a .set_configuration()). Improving the method's documentation and implementing different default initializations is another topic.

@acolomb acolomb closed this Jul 2, 2024
@erlend-aasland
Copy link
Contributor

FTR, I'd like to take a stab at improving the docs; it is a good opportunity for me to get to know the various APIs of this excellent library better.

Cookie-licking disclaimer: if I don't post a PR within a day or two, feel free to beat me to it :)

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.

4 participants