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

[VT] Wait on response before processing next command #380

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

GwnDaan
Copy link
Member

@GwnDaan GwnDaan commented Dec 7, 2023

To comply with the standard, we must wait for responses from the VT server before sending the next command. This PR implements that behaviour

@GwnDaan GwnDaan added the iso: virtual terminal Related to the ISO-11783:7 standard label Dec 7, 2023
@GwnDaan GwnDaan requested a review from ad3154 December 7, 2023 20:52
@GwnDaan GwnDaan self-assigned this Dec 7, 2023
@GwnDaan GwnDaan force-pushed the daan/wait-command-response branch from 1076417 to 3334b5a Compare December 8, 2023 09:02
@GwnDaan GwnDaan force-pushed the daan/wait-command-response branch 2 times, most recently from 9abf633 to c156d64 Compare December 10, 2023 09:22
@GwnDaan
Copy link
Member Author

GwnDaan commented Dec 10, 2023

The main branch is currently failing to build on PlatformIO: https://github.com/Open-Agriculture/AgIsoStack-plus-plus/actions/runs/7147251833/job/19466433156. This PR will also fix that

@ad3154
Copy link
Member

ad3154 commented Dec 10, 2023

Tested this by re-basing the seeder example on your branch, and... it's a little slow, only around 20 messages per second. Our update rate of the interface means that after a response is received we have to wait a fair bit before sending the next message which causes some noticeable lag. I am wondering if we should explore options to send the next message on-response.

Some ideas:

  • Mutex the queue and initiate the transmit of the next message from the receiving thread
  • Or, Use a condition variable to wake up the VT worker thread when a message response is received?
  • Update faster?
AgISOVirtualTerminal.2023-12-10.09-31-43.mp4

@GwnDaan GwnDaan force-pushed the daan/wait-command-response branch from c156d64 to 156843c Compare December 10, 2023 17:45
@GwnDaan
Copy link
Member Author

GwnDaan commented Dec 10, 2023

I opted for the mutex because I feel like it's the most efficient one, good that you tested this. I didn't have the environment to notice it

@GwnDaan GwnDaan force-pushed the daan/wait-command-response branch from 156843c to 1d2c3a7 Compare December 12, 2023 16:17
@GwnDaan GwnDaan force-pushed the daan/wait-command-response branch from 1d2c3a7 to d9f64ee Compare December 12, 2023 19:52
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

64.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@GwnDaan GwnDaan merged commit 97e6b36 into main Dec 13, 2023
9 of 10 checks passed
@GwnDaan GwnDaan deleted the daan/wait-command-response branch December 13, 2023 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iso: virtual terminal Related to the ISO-11783:7 standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants