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

SARA-R5 Support for retain in MQTT #136

Closed
michaelboeding opened this issue Sep 20, 2023 · 40 comments
Closed

SARA-R5 Support for retain in MQTT #136

michaelboeding opened this issue Sep 20, 2023 · 40 comments

Comments

@michaelboeding
Copy link

 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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

RobMeades commented Sep 20, 2023

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
Copy link
Author

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
Copy link
Author

michaelboeding commented Sep 23, 2023

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
Copy link

philwareublox commented Sep 23, 2023 via email

@michaelboeding
Copy link
Author

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
Copy link
Contributor

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

@michaelboeding
Copy link
Author

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
Copy link
Contributor

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

@michaelboeding
Copy link
Author

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

@michaelboeding
Copy link
Author

michaelboeding commented Sep 23, 2023

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
Copy link
Contributor

RobMeades commented Sep 23, 2023

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
Copy link
Author

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

@RobMeades
Copy link
Contributor

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

@michaelboeding
Copy link
Author

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
Copy link
Contributor

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
Copy link

philwareublox commented Sep 23, 2023 via email

@michaelboeding
Copy link
Author

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
Copy link
Author

michaelboeding commented Sep 23, 2023

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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

@RobMeades
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

"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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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

@RobMeades
Copy link
Contributor

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
Copy link
Author

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

@michaelboeding
Copy link
Author

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
Copy link
Contributor

RobMeades commented Sep 23, 2023

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
Copy link
Author

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
Copy link
Contributor

The change to allow clearing of retained MQTT messages is now pushed to master here, see a06de9f.

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

@michaelboeding
Copy link
Author

Moving over to it now

@michaelboeding
Copy link
Author

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

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

No branches or pull requests

3 participants