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

START_SYNC errors after command sequence indicated by pluggable discovery specification #1653

Closed
per1234 opened this issue Feb 4, 2022 · 5 comments · Fixed by #1749
Closed
Assignees
Labels
conclusion: resolved Issue was resolved topic: documentation Related to documentation for the project type: imperfection Perceived defect in any part of project

Comments

@per1234
Copy link
Contributor

per1234 commented Feb 4, 2022

Describe the bug

From the Pluggable Discovery Specification:

https://arduino.github.io/arduino-cli/dev/pluggable-discovery-specification/#start-command

The START command initializes and starts the discovery internal subroutines. This command must be called before LIST or START_SYNC.

To me, this indicates that in order to execute a START_SYNC, I should run the following sequence of commands:

  1. HELLO
  2. START
  3. START_SYNC

🐛 The sequence of commands indicated by the specification results in an error from the START_SYNC command.

To Reproduce

$ git clone https://github.com/arduino/pluggable-discovery-protocol-handler

$ cd pluggable-discovery-protocol-handler/dummy-discovery/

$ go build

$ ./dummy-discovery
HELLO 1 "arduino-cli"
{
  "eventType": "hello",
  "message": "OK",     
  "protocolVersion": 1 
}
START
{
  "eventType": "start",
  "message": "OK"      
}
START_SYNC
{
  "eventType": "start_sync",
  "message": "Discovery already STARTed, cannot START_SYNC",
  "error": true
}

Expected behavior

Specification to be corrected if the behavior of the github.com/arduino/pluggable-discovery-protocol-handler/v2 module is what is intended, which would entail one of the following:

  • Remove the statement that START is required before START_SYNC
  • Specify that STOP must be called before START_SYNC

-OR-

The github.com/arduino/pluggable-discovery-protocol-handler/v2 module to support the command sequence indicated in the specification.

Desktop

  • OS: Windows 10, Ubuntu 20.04
  • github.com/arduino/pluggable-discovery-protocol-handler/v2@a0d87f340f91cef6454ea0601f55d1119a5a8bb7
@per1234 per1234 added the type: imperfection Perceived defect in any part of project label Feb 4, 2022
@cmaglie
Copy link
Member

cmaglie commented Feb 4, 2022

Actually the discovery it's working correctly, this is an error in the specification (I think it's a leftover from the RFC).

The state machine here https://arduino.github.io/arduino-cli/dev/pluggable-discovery-specification/#state-machine shows the allowed state transitions and there is no direct transition from START to START_SYNC.

@per1234 per1234 transferred this issue from arduino/pluggable-discovery-protocol-handler Feb 4, 2022
@per1234
Copy link
Contributor Author

per1234 commented Feb 4, 2022

Thanks for taking a look Cristian. I transferred the issue to the Arduino CLI repo since it is now clear that the defect must be corrected in this repository.

@per1234 per1234 assigned per1234 and unassigned cmaglie Feb 4, 2022
@per1234 per1234 added the topic: documentation Related to documentation for the project label Feb 4, 2022
@per1234
Copy link
Contributor Author

per1234 commented Feb 4, 2022

We will also need to resolve the other instances of the error that propagated from the specification to the documentation of other projects:

I will propose that this be done by replacing the duplicate content in those repositories with a link to the Pluggable Discovery Specification document in order to avoid the need to maintain multiple copies.

@per1234 per1234 changed the title START_SYNC errors after command sequence indicated by specification START_SYNC errors after command sequence indicated by pluggable discovery specification Feb 4, 2022
@PaulStoffregen
Copy link
Sponsor

PaulStoffregen commented Feb 4, 2022

EDIT: deleted this comment and started a new issue #1654. It's really a different issue than the minor specification wording disagreement between the START command and State Machine portion of the spec.

@PaulStoffregen
Copy link
Sponsor

Also, if you're updating the spec, please consider the wording "A well behaved pluggable discovery tool must reflect the following state machine". Perhaps the word "must" really applies to the client which communicates with the discovery tool. The client is the entity which "must" obey this state machine (eg, only transmit START_SYNC while in the "Idling" state).

cmaglie added a commit to cmaglie/arduino-cli that referenced this issue Jun 3, 2022
@per1234 per1234 added the conclusion: resolved Issue was resolved label Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: resolved Issue was resolved topic: documentation Related to documentation for the project type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants