u-blox / ubxlib

Portable C libraries which provide APIs to build applications with u-blox products and services. Delivered as add-on to existing microcontroller and RTOS SDKs.
Apache License 2.0
303 stars 92 forks source link

SARA-R5 Support for retain in MQTT #136

Closed michaelboeding closed 1 year ago

michaelboeding commented 1 year ago
 bool retain;                       /**< if set to true then the topic
                                            subscriptions and message queue
                                            status will be kept by both the
                                            client and the broker across MQTT
                                            disconnects/connects. Defaults to
                                            false. The SARA-R5 cellular module
                                            does not support retention. */

Hey I was implementing a few things for MQTT for my project and noticed that the retain flag was not supported for the SARA-R5 modules. Is that something we can add? With my current structure it is pretty critical. Thanks!

RobMeades commented 1 year ago

Hi Michael: unfortunately it is not supported by the FW of the SARA-R5 module:

image

I don't believe there is any way we can add this at ubxlib level, it is something that the MQTT stack itself, which is all inside the SARA-R5 module, invokes through talking to the MQTT broker, unless you can think of a way?

michaelboeding commented 1 year ago

This is far fetched to ask them I guess but is there anyway the firmware team will add that support in a update anytime soon? I can't think of anything directly there to allow up but maybe linking the data connection to the PPPoS (ESP-IDF), and then using the network layer to use the MQTT client that Espressif offers?

RobMeades commented 1 year ago

We're just checking with the relevant developer as to why it is like it is, in case we've misunderstood something ourselves. Certification/approval cycles for cellular module FW are such that if the feature is not already available on an internal release branch somewhere, targeted for a maintenance release [which are at most annual] it is relatively unlikely to be arriving soon I'm afraid.

On the "using the platform's MQTT client" thing, the ubxlib code was always designed with the intention that we could interface into the platform's code just above their TLS; it is for this reason that u_sock.h was coded to look as much like LWIP as possible, since that's what pretty much every platform uses "down below".

This is now relatively high up our TODO list, but it would need to work with at least ESP-IDF and Zephyr and, of course, once you have such an integration you get caught up in the wheel-spooks of a couple of much larger threshing machines, which will bring entertainment for the future.

In summary: we are pretty likely to provide such an integration but I'd guess it would be unlikely to arrive this year.

Will update this issue when we have a response internally.

michaelboeding commented 1 year ago

Sounds good - again I appreciate the response. I was able to restructure some stuff and make my protocol work without retain for the device anyways but it does seem like a missing feature for a not so apparent reason (to me who knows nothing about the software running these things). The PPPoS would be awesome but its very complex to link up - I've seen a couple implementations so far and they are by far not simple.

michaelboeding commented 1 year ago

I actually lied I wasn't able to actually accomplish what I wanted with this. I don't need to publish a retained message but I need to clear one which requires you to send a NULL payload with the retain flag set. I'm pretty much out of ideas at this point. I guess I can always downgrade to a SARA-R4? Just seems like a bad solution.

Also looking at what you posted it looks like retain is allowed on the last will message #7? And the clean session #12 is not allowed?

philwareublox commented 1 year ago

The SARA-R5 doesn't support the "clean session" option - as it always cleans the session on connection to the broker. The MQTT client we use has some limitation on this as the session isn't just about the broker, but the client too. Currently it always cleans the session on connection.

However, the 'retain' option of the MQTT message should be working. This is just a flag on the AT command. I will check with Rob about ubxlib and this support.

Can you describe what you are doing fully - just to make sure we understand your application.

michaelboeding commented 1 year ago

Thanks for the reply Phil,

Clean session isn't a huge problem - I can work around that portion, thanks for the explanation.

As for the retain functionllity I am basically sending settings to a topic from a mobile application with the retain flag set - so that the device gets the setting immediately when it comes online. After my device gets the settings I want to clear the retain flag associated with that settings topic since I have already serviced it. That's where I run into problems, that doesn't seem to work through the cell MQTT client. I'm not actually sending a retained message to a topic with data but instead trying to clear the retained message currently on the broker. This all works correctly through my Wi-Fi MQTT client.

Heres some brief code - I can provide more if needed.

// Clear a retained message for a given topic
bool SARAR5::clearRetainedMessage(const char *topic) {
    // Dummy empty message to clear the retained flag
    const char *emptyMessage = "";
    // Publish an empty message with the retain flag set to true
    int32_t errorCode = uMqttClientPublish(pContext, topic, emptyMessage, 
                                            strlen(emptyMessage),
                                            U_MQTT_QOS_AT_LEAST_ONCE, 
                                            true);
    if (errorCode == 0) {
        printf("Successfully cleared retained message for topic \"%s\"\n", topic);
        return true;
    } else {
        printf("Failed to clear retained message for topic \"%s\". Error code: %d\n", topic, errorCode);
        return false;
    }
}
RobMeades commented 1 year ago

FYI, not that it matters I think, but the retain flag is exposed as a parameter of uMqttClientPublish().

michaelboeding commented 1 year ago

FYI, not that it matters I think, but the retain flag is exposed as a parameter of uMqttClientPublish().

Yes, I believe I'm using that in the last parameter. I think the error I get is -9? But Ill need to go back and reproduce it again to be sure...it was late last night when I was testing that.

RobMeades commented 1 year ago

If that's a -9 return code from a ubxlib API it is U_ERROR_COMMON_TIMEOUT.

michaelboeding commented 1 year ago

Let me spin it up and test now to get the actually result. One second

michaelboeding commented 1 year ago

That's what I get for trying to remember something when sleep deprived...the correct error is -5.

Failed to clear retained message for topic "SC/94b97e7b8f50/cloud/fences". Error code: -5
Failed to clear retained message for topic "SC/94b97e7b8f50/cloud/fences"

Which looks to be a U_ERROR_COMMON_INVALID_PARAMETER = U_ERROR_BASE - 5, error

RobMeades commented 1 year ago

Ah, yes, we don't allow empty messages in the uMqttClientPublish() command, so this I could do something about. Thing is, I wonder why that code is there?

Lemme try adding a test for it with the check removed and see if anything falls over in a heap.

michaelboeding commented 1 year ago

Thanks again for the support guys....I know its Saturday feel free to leave this stuff until Monday....

RobMeades commented 1 year ago

Nah, we live the rock 'n roll life-style :-).

michaelboeding commented 1 year ago

Ah, yes, we don't allow empty messages in the uMqttClientPublish() command, so this I could do something about. Thing is, I wonder why that code is there?

Lemme try adding a test for it with the check removed and see if anything falls over in a heap.

Sending a blank message to any other topic without the retain flag with a blank message would be pointless imo. But when trying to clear a retained flag you have to do that or NULL.

Heres my esp-mqtt client code to clear it

//used to clear a retained message on a topic
void WifiLayer::clearRetainedMessage(esp_mqtt_client_handle_t client, const char* topic) {
    int msg_id = esp_mqtt_client_publish(client, topic, NULL, 0, 0, 1);
    if (msg_id == -1) {
        printf("Failed to clear retained message\n");
    } else {
        printf("Retained message cleared successfully, msg_id=%d\n", msg_id);
    }
}
RobMeades commented 1 year ago

For my clarification: in the case where you get U_ERROR_COMMON_INVALID_PARAMETER, are you sending an empty string (i.e. just a terminator, 0) or a message of length zero? I guess the latter?

philwareublox commented 1 year ago

Just to confirm, the purpose here is to clear a previous retained message?

michaelboeding commented 1 year ago

Just to confirm, the purpose here is to clear a previous retained message?

Yes - I'm attempting to clear the settings topic from the broker since my device got it and serviced it.

michaelboeding commented 1 year ago

For my clarification: in the case where you get U_ERROR_COMMON_INVALID_PARAMETER, are you sending an empty string (i.e. just a terminator, 0) or a message of length zero? I guess the latter?

  // This will clear any previous retained messages for that topic.
    if (this->saraR5->clearRetainedMessage(topic)) {
        printf("Successfully cleared retained message for topic \"%s\"\n", topic);
    } else {
        printf("Failed to clear retained message for topic \"%s\"\n", topic);
    }

// Clear a retained message for a given topic
bool SARAR5::clearRetainedMessage(const char *topic) {
    // Dummy empty message to clear the retained flag
    const char *emptyMessage = "";
    // Publish an empty message with the retain flag set to true
    int32_t errorCode = uMqttClientPublish(pContext, topic, emptyMessage, 
                                            strlen(emptyMessage),
                                            U_MQTT_QOS_AT_LEAST_ONCE, 
                                            true);
    if (errorCode == 0) {
        printf("Successfully cleared retained message for topic \"%s\"\n", topic);
        return true;
    } else {
        printf("Failed to clear retained message for topic \"%s\". Error code: %d\n", topic, errorCode);
        return false;
    }
}

Looks like im sending an empty string "". Maybe we can do the same thing the esp-mqtt library does and accept NULL?

RobMeades commented 1 year ago

I've checked the Wifi code (which the uMqttClient API also works with) and it doesn't like being given an empty message but I'm also not sure that there is a real reason for that either. I will try removing that check also.

michaelboeding commented 1 year ago

Maybe we can replicate what the esp-mqtt client does and leave your empty message checks but allow NULL to be passed into both the length and message to signify you are clearing a retained message? Not sure just an idea.

RobMeades commented 1 year ago

That's an interesting idea: I will try that.

RobMeades commented 1 year ago

For my benefit (I'm no MQTT expert), how does the broker know which message is to be not-retained, or does it just effectively clear the topic?

michaelboeding commented 1 year ago

Yeah so you can only have one retained message on a topic. So there's only ever one settings topic that should be retained (which would be the last one sent from the mobile device). So when you send the NULL or blank message to the broker it will then just clear it. If that makes sense?

Like in my example I have the mobile app call a cloud function to effectively set the settings message I want on the device with a retain flag set.

RobMeades commented 1 year ago

Cool, I didn't know that; I think you are right that a backwards-compatible way to do this would be to remove the check for NULL/zero-length on a uMqttClientPublish() if retain is false, and I can explain the functionality you describe in the function header. Let's see if the Wifi code likes it.

michaelboeding commented 1 year ago

Brokers will need the retain message to be set and a blank string or NULL sent to the broker to remove the retained topic/message. So I may be misunderstanding what you said but I think you want the NULL/zero length checks to still be in place for when the retain flag is false but disabled when retain is true. Another option is to directly bake in a new method called clearRetainedMessage or something like that.

RobMeades commented 1 year ago

Ah, sorry, yes, I had misunderstood. Seem more like a hack in the MQTT specs now :-). It's less code to add this to the existing API so let's stick with that and explain it well.

michaelboeding commented 1 year ago

"By default, every MQTT topic can have a retained message. The standard MQTT mechanism to clean up retained messages is sending a retained message with an empty payload to a topic. This will remove the retained message."

RobMeades commented 1 year ago

This is now fixed and passes testing but I'll need to get it reviewed on Monday before I can push it to master so, until then, find here:

https://github.com/u-blox/ubxlib/tree/preview_fix_mqtt_retain_rmea

...a preview branch that you can use. I will update this issue when the change is pushed to master and will delete the preview branch a little after that.

michaelboeding commented 1 year ago

Awesome thanks so much Rob!!!! Perfect timing I just got done writing all the code to get info from the cell module and gps module to save to the database. Again awesome support!

michaelboeding commented 1 year ago

Is the fix just passing the "" to the publish with retain set or a NULL?

RobMeades commented 1 year ago

In implementation terms, the cellular module interface won't let me send a binary message with zero length, so I tell it to use ASCII mode and send it "", which I hope will do what is required:

AT+UMQTTC=2,2,1,0,"ubx_test/352709570012633",""

If that does not work for the broker I can try changing the code to send in HEX mode and send zero bytes instead?

michaelboeding commented 1 year ago

Let me pull that now and give it a try and let you know. Thanks

michaelboeding commented 1 year ago

Looks like it works on initial testing!!! 👌🏻. My retained message in AWS IoT core seems to be cleared. Side question now - does this also now allow the SARA-R5 to publish retained messages? Like I said not a requirement for me but some systems rely on them heavily from the device to server. Might be a good feature to have.

RobMeades commented 1 year ago

does this also now allow the SARA-R5 to publish retained messages?

It should have always been possible to publish a retained message, the issue with SARA-R5 was that it would clear them on disconnect, IYSWIM [and this it continues to do I'm afraid].

michaelboeding commented 1 year ago

Got it - sounds good. One thing I did notice is that I went into an endless loop when publishing to that retain topic but I'm adding a check to ignore if the payload for that topic is 0. So that should be client side - but if anyone runs into that I just wanted to comment it here. The endless loop is caused because im subscribed to that topic and then publish to it and then try to clear the retained message again etc.

RobMeades commented 1 year ago

The change to allow clearing of retained MQTT messages is now pushed to master here, see https://github.com/u-blox/ubxlib/commit/a06de9f29e124c80d5f1c4d1d8c43284cddca500.

If you can confirm here when you have moved over to that I will delete preview_fix_mqtt_retain_rmea.

michaelboeding commented 1 year ago

Moving over to it now

michaelboeding commented 1 year ago

Pulled the new branch - closing this issue. Thanks for the fix!