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

[TC]: Add options for falling back to other language data sources when TC doesn't respond #448

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

ad3154
Copy link
Member

@ad3154 ad3154 commented Mar 13, 2024

Describe your changes

This change alters how we deal with the language command PGN in the TC client to be more reliable and more tolerant of TCs which don't fully comply with version 4 of the standard when they claim they're version 4.

  • Allow the TC client to fall back to other language sources as needed.
  • Fixed a condition where the TC client would become stuck while waiting for a language response if a response never arrived.
  • Added a getter to the language command interface to allow retrieving the assigned partner CF.
  • Prevent a crash in the language command interface which could occur if you send the command with some fields left unconfigured.
  • Removed TC client's dependency on the VT client.

How has this been tested?

This is pretty challenging to test in a unit test without a large amount of boiler plate code, so it has minimal testing at the moment. Ideally it would be nice to try this against a TC which doesn't send the language command. I would like to get some kind of automated test added though, so I've made this a draft until I can cover at least the VT fallback scenario.

Added unit test coverage of the fallback scenario and the 6 second complete timeout scenario.

@ad3154 ad3154 added enhancement New feature or request iso: task controller Related to the ISO-11783:10 standard labels Mar 13, 2024
@ad3154 ad3154 requested a review from GwnDaan March 13, 2024 01:35
@ad3154 ad3154 self-assigned this Mar 13, 2024
@ad3154 ad3154 force-pushed the adrian/tc-language-fallback branch 2 times, most recently from a4c3571 to 669028a Compare March 14, 2024 01:24
@ad3154 ad3154 marked this pull request as ready for review March 14, 2024 01:36
GwnDaan
GwnDaan previously approved these changes Mar 15, 2024
Copy link
Member

@GwnDaan GwnDaan left a comment

Choose a reason for hiding this comment

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

LGTM. I like how you separated the "languageCommandWaitingTimestamp_ms" and the "stateMachineTimestamp_ms" then there is no way of getting a livelock.

One thing to note, when we fallback onto the VT language interface, we don't store the result in the TCClient's languageCommandInterface. Might lead to inconsistencies in the application.

One thing I see how we could combat it; just set the partner of the languageCommandInterface of the TCClient to the VT control function, instead of doing requests via the commandInterface of the VTClient

EDIT: that could also remove the dependency on the VT, since it was only being used for the languageInterface inside the TC. Now we can just pass the partneredControlFunction of the VT to the TC client?

@ad3154
Copy link
Member Author

ad3154 commented Mar 15, 2024

EDIT: that could also remove the dependency on the VT, since it was only being used for the languageInterface inside the TC. Now we can just pass the partneredControlFunction of the VT to the TC client?

Oh hey, that sounds great!

I'll change that. Removing the dependency sounds really nice, I never really liked how that was....

ad3154 added 2 commits March 19, 2024 17:22
…n TC doesn't respond

Allow the TC client to fall back to other language sources as needed.
Fixed a condition where the TC client would become stuck while waiting for a language response if a response never arrived.
Added a getter to the language command interface to allow retrieving the assigned partner CF.
Prevent a crash in the language command interface which could occur if you send the command with some fields left unconfigured.
Removed references to the VT client in the TC client, since all it was
used for was the language command, which we can accomplish by only
passing the control function of the VT in instead.
Copy link

@ad3154 ad3154 merged commit ab2e8a9 into main Mar 22, 2024
10 checks passed
@ad3154 ad3154 deleted the adrian/tc-language-fallback branch March 22, 2024 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request iso: task controller Related to the ISO-11783:10 standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants