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

examples/simple_ds402_node.py uses contradictory PDO mapping patterns #496

Open
acolomb opened this issue Jul 4, 2024 · 7 comments
Open

Comments

@acolomb
Copy link
Collaborator

acolomb commented Jul 4, 2024

The example uses the RemoteNode.load_configuration() API to send ParameterValue settings given in the sample EDS file to the device. This used to work in conjunction with PdoBase.read() to synchronize the locally cached PDO configuration and mapping info. Calling that method (on any PDO) was therefore not really necessary, as it was implied as a final step in load_configuration().

But #427 changed this and made load_configuration() apply the PDO configuration through calls to PdoBase.save() instead. Thus the need to read it back was eliminated.

In any case, the example goes on to call BaseNode402.setup_402_state_machine(), which then reads back the PDO configuration again. Here it would help to pass read_pdos=False since we know that the configuration was just saved, skipping the unnecessary upload.

That convenience method also checks whether needed values like the status and control words are configured in the respective PDOs. Which should succeed because of the initialization values in the sample EDS (haven't tested).

But then the example code continues with re-mapping the PDOs, which (probably?) invalidates the rpdo_pointers and tpdo_pointers entries pointing at the previous PDO mapping's PdoVariable objects. This should be done before setup_402_state_machine() to avoid this error. In addition, the example first uses node.tpdo.read() and node.rpdo.read() before adjusting the mappings, which, again, is unnecessary after load_configuration().

Furthermore, before the PDO re-mapping, it tries to change the node.state attribute, but there is no prior change of node.nmt.state = 'OPERATIONAL', thus the exchanges of control and status words cannot work. There is a fallback to SDO implemented if no PDO maps the required objects, but that is not triggered because they are in fact mapped. The problem is that in PRE-OPERATIONAL state, the node's PDOs simply do not function. Re-mapping them after the previous PdoVariable objects have been cached (see above) doesn't make this any better.

These are some observations from trying to understand the sample code, showing that it is mostly broken for actual usage and suggests wrong or inefficient patterns of the library API usage. There may be more problems hidden, which could be revealed after testing with actual hardware. Maybe that was done at some point, but probably not after the library API has evolved and some functionality (especially regarding profile CiA402) moved around.

We should discuss how to make this example actually useful again, or at the very minimum, document that it serves only as a demonstration of the API and will not work with real hardware unless the user takes care of the issues with their hopefully firm CANopen background themselves.

@erlend-aasland
Copy link
Contributor

We should discuss how to make this example actually useful again, or at the very minimum, document that it serves only as a demonstration of the API and will not work with real hardware unless the user takes care of the issues with their hopefully firm CANopen background themselves.

FWIW, I think it would make sense to convert it to a full-fledged tutorial, alternatively a more concise how-to.

@sveinse
Copy link
Collaborator

sveinse commented Jul 6, 2024

The way a user needs to setup any node is highly dependent on the node capabilities.

  • Some node might not have any PDO configuration on startup and needs .load_configuration().
  • Some nodes have static PDO configuration and the local PDO configuration must be loaded from the OD (pdo.read(from_od=True)).
  • Or perhaps the node has a variable PDO (compared to the OD) and it needs to be fetched from the node before use (.pdo.read(from_od=False)).
    I think the examples should highlight these three use cases

@erlend-aasland
Copy link
Contributor

This sounds like a series of How-to guides to me.

@romainreignier
Copy link

I wanted to try the 402 example and faced the same issue. Do you guys have a working example for 402?

@acolomb
Copy link
Collaborator Author

acolomb commented Oct 2, 2024

Sorry, nobody has taken the time to revamp this example yet. I do not have any off-the-shelf working script for CiA 402 operation.

My advice currently is to understand the spec, figure out what configuration you actually want to achieve and then pick out the API calls necessary to achieve that. It should be rather simple with the convenience methods provided, but you probably need to figure out a sensible PDO mapping first. Or see what the device manufacturer has as a default mapping, that usually works quite well if the device advertises 402 compliance.

Of course I understand that a ready-to-run generic example will be helpful as a starting point. Maybe you can come up with such a simple script after figuring it out for yourself? Then we could integrate that here. It's always better to write "beginner's" documentation after actually working with someone less experienced. If I were to write a beginner's tutorial, I'd need to pretend not knowing what I do know, which often misses the goal.

@romainreignier
Copy link

Thank you for your reply.
I have managed to make my motor move with the base canopen.RemoteNode and writing to the ControlWord 0x6040.
When I try to use the example, I keep having this log:

The object 0x6041 is not a configured TPDO, fallback to SDO

I have tried to both modify the EDS to set the mapping and set the mapping in the code for the warning is still here.

I the code, I have written this, following the PDO documentation:

node.tpdo.read()
node.tpdo[1].clear()
node.tpdo[1].add_variable("Statusword")
node.tpdo[1].add_variable("Position actual value")
node.tpdo[1].trans_type = 254
node.tpdo[1].event_timer = 100
node.tpdo[1].enabled = True
node.tpdo[2].clear()
node.tpdo[2].add_variable("Modes of operation")
node.tpdo[2].add_variable("Modes of operation display")
node.tpdo[2].trans_type = 254
node.tpdo[2].event_timer = 100
node.tpdo[2].enabled = True
node.tpdo.save()

Does this look ok for you?

@acolomb
Copy link
Collaborator Author

acolomb commented Oct 2, 2024

Shouldn't the Modes of operation object be mapped to an RPDO? Better to use numeric references here, as we don't know what your EDS files names each parameter. If you are in fact not calling any code related to RPDO, then those will have your device's default mappings. Check and see what those are as well.

In general, you need to call the setup_402_state_machine() method after configuring and saving you PDO mappings. If you've saved them via the code above (and not just store the config in the device's persistent memory), then you can pass read_pdos=False to that method, to skip reading back (uploading) everything you just happened to save (download).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants