Taking ownership of telemetry client for MQTT requests

[pmldrmota]

Motivation

In one of our Stabilizers we needed a custom request-response topic for on-demand read access to a value (in this case the output of a filter).

Limitation of the current design

I believe the telemetry client is the right place for this, but the current implemetation (forwarding to mqtt.poll() in NetworkUsers.update()) makes it impossible to use transparently. After all, we not only want to add a custom callback to mqtt.poll(callback), but we also want to handle the Result of this call.

Proposed design

To keep the changes to existing applications minimal, I wrap it (similar to the FrameGenerator) in an Option and let NetworkUsers manage it depending on whether or not it contains a value. That way, we can take() the TelemetryClient and call mqtt.poll() to handle any custom requests on topics we previously subscribed to (using the mutable reference provided by a new method). The then-owner is responsible for calling mqtt.poll() regularly, for reasons stated in the TelemetryClient.update() docs.

Discussion

I'd be interested to know if you already had a design for this use case. I tested this one and it works nicely.

[pmldrmota]

Thank you for looking at this!

It seems to me like it would be more sound to accept a f: FnOnce(client, topic, payload, properties) function that gets passed to telemetry.rs:update().

This is of course also what I implemented at first, but realised that it does not allow the user to handle errors.

After all, we not only want to add a custom callback to mqtt.poll(callback), but we also want to handle the Result of this call.

For example would I want to try to resubscribe to a topic after the connection failed.

Are there problems with releasing a reference to the Minimq instance (TelemetryClient.mqtt) for the user to subscribe, publish, and finally poll manually?

[pmldrmota]

Just to explain what I'm after here: What I think we need is

• call Minimq::poll() and handle the result
• subscribe to custom topics using the MqttClient

this is a shortened version of our use case

fn idle(mut c: idle::Context) -> ! {
    // Keep track of subscription status
    let mut subscribed = false;
    // Defina a custom topic
    const custom_topic: &str = "dt/sinara/custom"; 

    loop {
        // Lock the network
        match c.shared.network.lock(|net| {
            let mqtt_result = {
                // !Get access to the Minimq MQTT client!
                let mqtt = net.telemetry.mqtt();

                // Subscribe to the custom topic if we haven't already
                if !subscribed {
                    if let Ok(_) = mqtt.client.subscribe(&custom_topic, &[]) {
                        subscribed = true;
                    }
                }

                // Poll for new incoming messages under the custom topic
                // and immediately respond using the client provided.
                mqtt.poll(|client, topic, _message, properties| {
                    let payload = if topic == custom_topic {
                        "Custom payload".as_bytes()
                    } else {
                        "Unexpected topic".as_bytes()
                    };
                    // respond to the message
                    client.publish("dt/sinara/response_custom", payload).ok();
                })
            };
            // Handle errors from custom topic handling
            if let Err(_) = mqtt_result {
                subscribed = false;
            }
            // Update all other network users
            net.update_all_but_telemetry()
        }) {
            // Handle the usual Miniconf cases
            NetworkState::SettingsChanged(_path) => {
                settings_update::spawn().unwrap()
            },
            NetworkState::Updated => {},
            NetworkState::NoChange => cortex_m::asm::wfi(),
        }
    }
}

[ryan-summers]

Are there problems with releasing a reference to the Minimq instance (TelemetryClient.mqtt) for the user to subscribe, publish, and finally poll manually?

As far as I'm aware, exposing mutable access to the TelemetryClient would be perfectly acceptable, as long as it remains within the NetworkUser struct (since this implies RTIC will continue managing resource access properly).

You've also given me some interesting thoughts in how we can improve Minimq (MQTT client)'s usability. I'll take a look to see if we can support use-cases like this a bit easier.

[nkrackow]

There were also some discussions/changes in miniconf to allow for responses to settings updates. But I don't think you can respond with arbitrary data to a settings update right? https://github.com/quartiq/miniconf/pull/63

[ryan-summers]

There were also some discussions/changes in miniconf to allow for responses to settings updates. But I don't think you can respond with arbitrary data to a settings update right? quartiq/miniconf#63

I was thinking about that as well, but this isn't quite the same as miniconf - this isn't responding to a settings change, but rather collecting telemetry on-demand. The concept is very similar to that PR though

[jordens]

There is also https://github.com/quartiq/minireq for request/response. Unsure whether it is the right way to add this to telemetry.

[pmldrmota]

I just updated this PR with the solution I'm currently running after @ryan-summers pointed out that taking ownership of the telemetry client is profoundly unsound.

There is also https://github.com/quartiq/minireq for request/response.

That is very interesting! This is in fact exactly what we are doing (in an ad-hoc way for our specific application). I will have a look how this can be interfaced with the telemetry client.

[jordens]

It doesn't need to there is little real downside in having multiple MQTT clients. In fact it nicely takes care of the routing you'd have to do yourself otherwise.

[ryan-summers]

My only concern is that we don't currently expose a way for external users of stabilizer to add to NetworkUsers without modifying the stabilizer library. I think minireq is the right idea (I had completely forgotten about it), but we need to think about how to integrate this in properly as well.