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

Fix race condition skipping reference velocity update #85

Merged
merged 2 commits into from
Mar 19, 2020

Conversation

at-wat
Copy link
Member

@at-wat at-wat commented Mar 18, 2020

No description provided.

@at-wat at-wat self-assigned this Mar 18, 2020
@at-wat
Copy link
Member Author

at-wat commented Mar 19, 2020

@seiga-k please take a look.

@@ -109,7 +109,7 @@ void ISR_VelocityControl()
motor[i].ref.vel_diff = 0;
}

motor[i].error = motor[i].ref.vel_buf - motor[i].vel;
motor[i].error = motor[i].ref.vel - motor[i].vel;
Copy link

Choose a reason for hiding this comment

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

It seems better to leave this line as an old one.
Because if the interrupt occurs between these two lines https://github.com/T-frog/tf-2md3-firmware/pull/85/files#diff-bc1342d54328fc8b17402447f259b6dbR1529-R1530 , motor[i].ref.vel will not update but use new reference velocity.

Copy link
Member Author

Choose a reason for hiding this comment

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

@seiga-k I think ref.vel can be updated as soon as possible.
Since ref.vel_changed flag is used to calculate acceleration of the reference for feed-forward dynamics compensation, it's not matter if ref.vel_buf has single step delay against ref.vel.
What do you think?

Copy link

Choose a reason for hiding this comment

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

Yes, it makes only a single step delay.
I thought the priority of the matching the ref.vel and ref.vel_diff is higher than the update delay.
This is only a balance of priority. I understood your opinion now, so I can agree with you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another reason is that I would like to minimize possibility to miss updating the reference velocity, even if any other bugs are injected around ref.vel_changed.

@at-wat at-wat merged commit f8c0311 into master Mar 19, 2020
@at-wat at-wat deleted the fix-ref-vel-update-race branch March 19, 2020 07:35
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