Skip to content

Conversation

@brentru
Copy link
Member

@brentru brentru commented Nov 9, 2023

WipperSnapper API V2 aims to address:

  • Enforce consistent naming of messages and fields
  • Reduce the amount of MQTT topics utilized by WipperSnapper from >25 to 1 bi-directional topic, signal
  • Implement a unified sensor API, sensor.proto
  • Refactor pin.proto into analogio.proto and analogio.proto
  • Possible: Move to period-per-component (vs -per-sensor)

@brentru brentru requested review from lorennorman and tyeth November 9, 2023 19:09
Copy link
Contributor

@lorennorman lorennorman left a comment

Choose a reason for hiding this comment

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

This is all just really great! I had lots of notes reading through this but overall it's an obviously-better direction, better organized, more bang-for-buck, easier and more performant to implement in code.

/**
* CreateDescriptionRequest identifies a device with Adafruit.io WipperSnapper.
*/
message CreateDescriptionRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to get to something better than "description" before we're done. Not sure what right now, depends whether we drop the response part. If it's just checking in with your Semver, maybe "checkin" or something.

*/
message CreateDescriptionResponse {
message CreateDescription {
Response response = 1; /** Specifies if the hardware definition exists on the server. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Noticing the "response" part, I guess even if we wanted to do away with this response entirely, we'd need a way to signal that the checkin failed in this way (board lookup failed). Maybe that's another Error message type and the broker can just send that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I2C is still so massive and complex! I have a lingering feeling that there's more useful refactoring to do here if we focus on it, perhaps during implementation or an early investigative spike.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Keeping this conversation open as a reminder

Comment on lines 68 to 71
//digitalio.proto
wippersnapper.digitalio.v1.DigitalIOEvent digitalio_event = 1;
// analogio.proto
wippersnapper.analogio.v1.AnalogIOEvent analogio_event = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we avoiding an *Added event for io components? I forget if we talked about that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure, what are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know the hardware side of course, but if you can definitely tell if you've successfully connected the selected component, then yeah I'd advocate we have AnalogIOAdded and DigitalIOAdded messages to fit the pattern and improve user experience.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lorennorman To clarify here - we want a new message called AnalogIOAdded. We are not changing AnalogIOAdd to AnalogIOAdded`.

Copy link

@PetteriAimonen PetteriAimonen left a comment

Choose a reason for hiding this comment

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

Overall, looks reasonable. I'd consider removing the deprecated fields completely at this point, as you are making such large API changes that no existing code will be compatible anyway.

Comment on lines +78 to +80
wippersnapper.digitalio.DigitalIOEvent digitalio_event = 10;
// analogio.proto
wippersnapper.analogio.AnalogIOEvent analogio_event = 20;
Copy link
Contributor

Choose a reason for hiding this comment

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

are we intentionally skipping the "added" responses for digital and analog?

Copy link
Member Author

Choose a reason for hiding this comment

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

@lorennorman Do you want these as analogio_event_added? We don't seem to appean _added onto any of the DeviceToBroker event messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants