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

API V2 #133

Draft
wants to merge 129 commits into
base: master
Choose a base branch
from
Draft

API V2 #133

wants to merge 129 commits into from

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.

@@ -10,36 +10,20 @@ import "nanopb/nanopb.proto";
* 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

proto/wippersnapper/pixels/v1/pixels.proto Outdated Show resolved Hide resolved
proto/wippersnapper/pwm/v1/pwm.proto Outdated Show resolved Hide resolved
proto/wippersnapper/signal/v1/signal.proto Outdated Show resolved Hide resolved
proto/wippersnapper/signal/v1/signal.proto Outdated Show resolved Hide resolved
proto/wippersnapper/i2c/v1/i2c.proto Outdated Show resolved Hide resolved
proto/wippersnapper/signal/v1/signal.proto Outdated Show resolved Hide resolved
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.

proto/wippersnapper/analogio/v1/analogio.proto Outdated Show resolved Hide resolved
proto/wippersnapper/analogio/v1/analogio.proto Outdated Show resolved Hide resolved
proto/wippersnapper/digitalio/v1/digitalio.proto Outdated Show resolved Hide resolved
proto/wippersnapper/digitalio/v1/digitalio.proto Outdated Show resolved Hide resolved
proto/wippersnapper/ds18x20/v1/ds18x20.proto Outdated Show resolved Hide resolved
proto/wippersnapper/sensor/v1/sensor.proto Outdated Show resolved Hide resolved
proto/wippersnapper/servo/v1/servo.proto Outdated Show resolved Hide resolved
proto/wippersnapper/signal/v1/signal.proto Outdated Show resolved Hide resolved
proto/wippersnapper/signal/v1/signal.proto Outdated Show resolved Hide resolved
proto/wippersnapper/uart/v1/uart.proto Outdated Show resolved Hide resolved
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