damienpontifex / azure-iot-sdk-rs

A rust library for connecting devices to Microsoft Azure IoT Hub
MIT License
14 stars 11 forks source link

Support PNP model IDs over MQTT #61

Closed laumann closed 3 years ago

laumann commented 3 years ago

Here we:

  1. Add a dtmi module that can validate a provided string as a DTMI
  2. Extend IoTHubClient::new() to accept an Option<Dtmi> that is used by the MQTT client transport. The HTTPS transport just ignores the argument

Extending the constructor function is a breaking change though, and if that's a concern then another way of constructing the IoTHubClient is needed, but keep in mind that the model ID argument is needed at initialization time for the MQTT transport.

laumann commented 3 years ago

@damienpontifex I tried to implement stricter support for DTMI, so it's not just a string floating around, but otherwise it achieves the same as https://github.com/omnioiot/azure-iot-sdk-rs/commit/682438feaf204d3f1b7af2132cb256b542865b9b

laumann commented 3 years ago

I considered only having this argument be available if certain feature were enabled/disabled like

pub async fn new(
    …
    #[cfg(not(feature = "https-transport"))]
    model_id: Option<Dtmi>,
)

we could even put it behind a pnp flag or something.

pub async fn new(
    …
    #[cfg(and(feature = "pnp", not(feature = "https-transport")))]
    model_id: Option<Dtmi>,
)
laumann commented 3 years ago

For reference, this document shows how model ID is supported in the Azure IoT C SDK.

damienpontifex commented 3 years ago

Sorry it's taken me a while to get to this.

What do you think about a different new with like new_pnp_client or something to keep the plain client simple if a developer has no idea what pnp is when using this? new would then call the new pnp anyway, but might keep the interface simple.

laumann commented 3 years ago

Sorry it's taken me a while to get to this.

Don't worry, I'm not in a hurry with this :-) I'd like to help improve the library as we use it (we'll want to try out the DPS functionality soon), but I'll work in our own fork, and just push things upstream so you can incorporate it at your own leisure.

What do you think about a different new with like new_pnp_client or something to keep the plain client simple if a developer has no idea what pnp is when using this? new would then call the new pnp anyway, but might keep the interface simple.

I actually considered introducing an IoTHubClientBuilder so when new configuration options are added, it's just a matter of adding a new method to a builder instead of adding a new constructor that users would have to switch to.

Either way, I'm in favor of not breaking the existing new() function, because downstream users will experience a breaking change when they update to a newer version of this dependency.

What do you think?

laumann commented 3 years ago

@damienpontifex So I finally got back to updating this. I implemented the new_with_pnp() as you suggested.

damienpontifex commented 3 years ago

Closing in favour of #64 as @laumann mentioned that supersedes this