Skip to content

Adjust pluggable monitor spec re: DESCRIBE command response to match library behavior #1738

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

Closed
3 tasks done
per1234 opened this issue May 18, 2022 · 2 comments · Fixed by #1748
Closed
3 tasks done
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 May 18, 2022

Describe the problem

The pluggable monitor specification states:

https://arduino.github.io/arduino-cli/dev/pluggable-monitor-specification/#describe-command

The enum types must have a list of possible values

The example response shown in the specification also uses values key names, so I don't think it is only a matter of incorrect formatting of "values"

🐛 github.com/arduino/pluggable-monitor-protocol-handler.monitor.PortParameterDescriptor.Values has a field tag value of value, instead of the specification compliant values:

https://github.com/arduino/pluggable-monitor-protocol-handler/blob/3dee1f8eddeb94f728c66645ec748da0501340e6/message.go#L38

To reproduce

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

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

$ git log -1 --oneline
3dee1f8 (HEAD -> main, origin/main, origin/HEAD) Merge pull request arduino/pluggable-monitor-protocol-handler#12 from per1234/issue-forms

$ go build

$ ./dummy-monitor
HELLO 1 "Arduino IDE"
{
  "eventType": "hello",
  "message": "OK",
  "protocolVersion": 1
}
DESCRIBE
{
  "eventType": "describe",
  "message": "OK",
  "port_description": {
    "protocol": "test",
    "configuration_parameters": {
      "echo": {
        "label": "echo",
        "type": "enum",
        "value": [
          "on",
          "off"
        ],
        "selected": "on"
      },
      "speed": {
        "label": "Baudrate",
        "type": "enum",
        "value": [
          "9600",
          "19200",
          "38400",
          "57600",
          "115200"
        ],
        "selected": "9600"
      }
    }
  }
}

🐛 There are non-compliant key names:

  • port_description.configuration_parameters.echo.value
  • port_description.configuration_parameters.speed.value

Expected behavior

DESCRIBE command response is specification compliant.

'github.com/arduino/pluggable-monitor-protocol-handler' version

3dee1f8

Additional context

No response

Issue checklist

  • I searched for previous reports in the issue tracker
  • I verified the problem still occurs when using the latest version
  • My report contains all necessary details
@cmaglie
Copy link
Member

cmaglie commented May 23, 2022

🤦🏼

We already have everything out using value.
Maybe it's better to "fix" the specification instead, because if someone already implemented a monitor: he surely used value instead of values otherwise arduino-cli won't accept it.

@per1234 per1234 transferred this issue from arduino/pluggable-monitor-protocol-handler May 23, 2022
@per1234
Copy link
Contributor Author

per1234 commented May 23, 2022

Thanks for the response @cmaglie. I had the same feeling, but decided it was better to report the issue from the perspective of the specification being canonical and anything which doesn't match the specification being non-compliant.

Since the resolution will be done by changing the specification content hosted in the arduino/arduino-cli repository, I have now transferred the issue there.

@per1234 per1234 added topic: documentation Related to documentation for the project and removed topic: code Related to content of the project itself labels May 23, 2022
@per1234 per1234 changed the title DESCRIBE command response is not specification compliant Adjust pluggable monitor spec re: DESCRIBE command response to match library behavior May 23, 2022
@per1234 per1234 self-assigned this May 29, 2022
cmaglie added a commit to cmaglie/arduino-cli that referenced this issue Jun 3, 2022
cmaglie added a commit that referenced this issue Jun 6, 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.

2 participants