Add gRPC interface for board list watcher #1048
Conversation
silvanocerza
force-pushed
the
scerza/grpc-board-list-watcher
branch
from
0204a43 to
ae0df34
Compare
|
|
||
| message BoardListWatchResp { | ||
| // Event type as received from the serial discovery tool | ||
| string event_type = 1; |
There was a problem hiding this comment.
Maybe an enum could help instead of the current "add" and "remove" strings.
There was a problem hiding this comment.
Yeah, didn't think about it.
| // Information about the port | ||
| DetectedPort port = 2; | ||
| // Eventual errors when detecting connected boards | ||
| string error = 3; |
There was a problem hiding this comment.
I am not a proto expert and I hope @cmaglie can help, but I would have expected to have something like:
message BoardListWatchResp {
oneof content {
DetectedChange change = 1;
string error = 2;
}
}
message DetectedChange {
enum ChangeType { ADDED = 0; REMOVED = 0; }
ChangeType type = 1;
DetectedPort port = 2;
}There was a problem hiding this comment.
Neither am I and that's why didn't about this approach, I like it though.
There was a problem hiding this comment.
I'm positive to move the pair (ChangeType, DetectedPort) into their own separate message.
I would change the naming, since we are now talking about Ports IMHO it should be:
message BoardListWatchResp {
oneof content {
DetectedPortEvent event = 1;
string error = 2;
}
}
message DetectedPortEvent {
enum Event { ADDED = 0; REMOVED = 0; }
Event action = 1;
DetectedPort port = 2;
}
(and at this point also we should probably go all-in and call it Port instead of DetectedPort, this is something that we must face once Pluggable Discovery is merged...)
I'm not sure about the oneof, there are other places where it may be useful to make the API more explicit, anyway when I tried it, IIRC, the generated API turned out to be very messy especially in languages without sum types like go, java, c++ where accessors (boilerplate) methods are added to handle it correctly. Let's evaluate if the pro (a cleaner proto API) is more valuable than the cons (a bigger "cognitive" load in the implementation).
There was a problem hiding this comment.
Thank you for your recommendations, @cmaglie. As I said, I am fine with this API now. It's your call whether you want to make the changes now or later.
|
I have a couple of questions regarding the behavior;
{
"type": "add",
"address": "/dev/cu.usbmodem14101",
"protocol": "serial",
"protocol_label": "Serial Port (USB)",
"boards": [
{
"name": "Arduino Portenta H7 (M7 core)",
"FQBN": "arduino-beta-development:mbed:envie_m7"
},
{
"name": "Arduino Portenta H7 (ThreadDebug)",
"FQBN": "arduino-beta-development:mbed:envie_m7_thread_debug"
},
{
"name": "Arduino Portenta H7 (M7 core)",
"FQBN": "arduino-beta:mbed:envie_m7"
}
]
}
{
"type": "remove",
"address": "/dev/cu.usbmodem14101"
}Does it mean that consumers of the gRPC API should move away from a board-driven approach, and track ports instead? In other words, from now on boards are always kind of an additional "meta" information of the detected port, and the ports are the first-class citizens of the CLI. Thank you! |
|
You see 3 boards because by searching in the locally installed platforms we find mutiple matches for the VID/PID combination probably. If it's better for you I could change the message sent so that the list of possible connected boards to that port is included, I'd have to keep track of them in the CLI though. |
|
Suggested changes to the gRPC interface will be made in a future PR, this is good enough for now. |
Please check if the PR fulfills these requirements
before creating one)
our contributing guidelines
Add a gRPC interface for the board list watch command.
gRPC consumers must poll the daemon to know which boards are connected or disconnected.
gRPC consumers now receive an event whenever a board is connected or disconnected.
No.
None.
See how to contribute