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

Added a short delay to ensure timely PDOs #111

Merged
merged 8 commits into from
Oct 30, 2024

Conversation

preston-rogers
Copy link
Contributor

It was found that some devices, specifically Platinum drives, would not transition into operational state. From debugging, research, and headaches, it was found that the issue was due to non-timely sending of PDOs. When PDOs are not sent at a timely rate, the slave will not transition to Operational mode.

This information is presented here:

OpenEtherCATsociety/SOEM#224

"Mostly a 1ms cycle time will work. When the slave sees enough LRW packets at a constant interval it will lock to that and release to Operational."

@preston-rogers preston-rogers self-assigned this Aug 13, 2024
@preston-rogers preston-rogers requested a review from a team as a code owner August 13, 2024 23:34
@dwai-wai dwai-wai mentioned this pull request Sep 30, 2024
Copy link
Contributor

@dwai-wai dwai-wai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I am just missing some information, but it's not clear to me why the logic within the while loop would result in a delay that would lead a successful state transition for actuators.

Has this been tested somewhere with positive results?

src/jsd.c Outdated

ecx_writestate(&self->ecx_context, 0);

int attempt = 0;
while (true) {
struct timespec current_time;
clock_gettime(CLOCK_REALTIME, &current_time);
if ((start_processdata_time.tv_nsec - current_time.tv_nsec)/1e3 > timeout_us) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In start_processdata_time.tv_nsec - current_time.tv_nsec)/1e3, doesn't this always evaluate to a negative number since you are updating current_time so it's increasing, but start_processdata_time is stale from when it was first measured in L168?

Is that your intent here to always go into the else clause and then do a nanosleep(&diff)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof I actually fixed this locally for Ernest's fork but never changed it here. I'll fix this now XD

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it!

@preston-rogers
Copy link
Contributor Author

I think I am just missing some information, but it's not clear to me why the logic within the while loop would result in a delay that would lead a successful state transition for actuators.

Has this been tested somewhere with positive results?

Hey @dwai-wai,
Yes this was tested on Ernest. Internally the slaves expect communication at a particular rate and leave operational mode if this exchange is not within a certain bound.

Copy link
Contributor

@dwai-wai dwai-wai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have one documentation nit left, but I think the code changes achieves what is recommended OpenEtherCATsociety/SOEM#224

MSG_DEBUG("Went over the loop period!");
}
else {
struct timespec diff;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: To help a future maintainer, I think it would be nice to add
"Do a nanosleep until the next tick as defined by timeout_us before attempting to do a receive_processdata so that we can send LRW packets at a constant interval to encourage a successful transition to OP state"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all LGTM, just need a codeowner's approval before merging (I am not a codeowner D: )

@preston-rogers preston-rogers merged commit 7ce3586 into master Oct 30, 2024
2 checks passed
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.

2 participants