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

Removed automatic change from NMT BOOTING to NMT PRE-OPERATIONAL #402

Closed

Conversation

Jef-GB
Copy link

@Jef-GB Jef-GB commented Oct 24, 2023

Description

Removed the if statement for automatically setting the NMT state to pre-operational when the node published a message with the value booting.

Changes

  • Remove auto change to Pre-Op from Booting

Linked Issue

Fixes #401

Removed the if statement for automatically setting the NMT state to pre-operational when the node published a message with the value booting.
@acolomb
Copy link
Collaborator

acolomb commented Apr 24, 2024

Why do you think this change is correct?

The CANopen standard clearly defines that a node enters the PRE-OPERATIONAL state automatically, right after sending its boot-up message (section 7.3.2.2.1). So I think the previous behavior is correct.

@acolomb
Copy link
Collaborator

acolomb commented Jul 4, 2024

Ping @Jef-GB, I'm about to close this as I don't see any wrong behavior to fix here. Unless you have a good explanation why it's necessary.

@erlend-aasland
Copy link
Contributor

The CANopen standard clearly defines that a node enters the PRE-OPERATIONAL state automatically, right after sending its boot-up message (section 7.3.2.2.1). So I think the previous behavior is correct.

What about non-compliant and buggy nodes? Would it not make sense to just await the PRE-OPERATIONAL heartbeat instead?

@acolomb
Copy link
Collaborator

acolomb commented Jul 10, 2024

It's perfectly normal for a device to not send any more NMT messages after its boot-up. Heartbeat needs to be enabled explicitly. Assuming standard-compliant behavior by default is the Right Thing to do, any known buggy devices could be handled by subscribing via add_heartbeat_callback() and tracking state manually.

@erlend-aasland
Copy link
Contributor

It's perfectly normal for a device to not send any more NMT messages after its boot-up.

In that case, there's no need for this change. The docs could be made a little bit more explicit about this, though.

@friederschueler
Copy link
Collaborator

First, can we please move the discussion to the #401 issue and we should close this PR.

Reason: Default behavior change might break existing applications.

But I also see the need to have a "synchronized" state hassle-free. Maybe we can introduce some option for the node, which enables state transitions based on the heartbeat only. Because I think it is a very commong use-case to wait for a device until it finished its internal startup. Currently in my older mc-test projects I use a fixed waiting time, which is bad. In the newer tests, I subscribe to the heartbeat to do another is-ready check, but this would be so much easier if I just could enable this behaviour on the node itself.

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.

Can't wait for RemoteNode to reach PRE-OPERATIONAL
4 participants