Skip to content

Conversation

Fapiko
Copy link
Contributor

@Fapiko Fapiko commented Feb 20, 2020

This adds retains support to published messages. It should be backwards compatible with existing clients as it defaults to false if not supplied.

Solves #20 and #161

Copy link
Contributor

@flavio-fernandes flavio-fernandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline. otherwise, LGTM

Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fapiko Thanks for making the changes, I'm excited to merge this in. I do have a few questions.


bool Adafruit_MQTT::publish(const char *topic, const char *data, uint8_t qos) {
return publish(topic, (uint8_t*)(data), strlen(data), qos);
bool Adafruit_MQTT::publish(const char *topic, const char *data, uint8_t qos, bool retain) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like the avoid changing function signatures, is there a way to update this method without changing the signature?

Copy link
Contributor Author

@Fapiko Fapiko Feb 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set the retain parameter to default to false in the header definitions for each of these methods so I believe this should be backwards compatible with existing clients. Please correct me if that assumption is false though as I don't have a ton of experience with C++

Copy link
Contributor

@flavio-fernandes flavio-fernandes Feb 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a bunch of c++, so let me chime in on this one. ;)
I see two options here:

  1. make retain an optional parameter : retain=false
  2. create a new method, so the existing one remains undisturbed

See this link for more info on option 1

Option 2 may be the cleanest, so the caller does not get confused about the implicit parameter.
What that means is that you would add a new public function called something like:

bool Adafruit_MQTT::publishWithRetain(const char *topic, const char *data, uint8_t qos, bool retain) {
  // code that was originally in Adafruit_MQTT::publish goes here plus
  // handling of the retain parameter.
}

bool Adafruit_MQTT::publish(const char *topic, const char *data, uint8_t qos) {
  // all this function does now is to call the new one, with the default retain value
  return publishWithRetain(topic, data, qos, false /*retain*/);
}

Hope this makes sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: after looking that the existing code, it seems to me that a better approach for this
may be to simply overload publish, as it is currently done for publish that does not use "uint16_t bLen".
Sorry for not seeing that sooner. ;)

In other words:

  // Publish a message to a topic using the specified QoS level.  Returns true
  // if the message was published, false otherwise.
  bool publish(const char *topic, const char *payload, uint8_t qos = 0);
  bool publish(const char *topic, uint8_t *payload, uint16_t bLen, uint8_t qos = 0);
  bool publish(const char *topic, /*const*/ uint8_t *payload, uint16_t bLen, uint8_t qos, bool retain);   // ADD THIS
bool Adafruit_MQTT::publish(const char *topic, const char *data, uint8_t qos) {
  return publish(topic, (uint8_t*)(data), strlen(data), qos, false);
}

bool Adafruit_MQTT::publish(const char *topic, uint8_t *data, uint16_t bLen, uint8_t qos) {
  return publish(topic, data, bLen, qos, false);
}

bool Adafruit_MQTT::publish(const char *topic, /*const*/ uint8_t *data, uint16_t bLen, uint8_t qos, bool retain) {
  // Construct and send publish packet.
  uint16_t len = publishPacket(buffer, (uint16_t) sizeof(buffer), topic, data, bLen, qos /*, ADD retain HERE...*/);
  if (!sendPacket(buffer, len))
    return false;
...

}

bool Adafruit_MQTT::publish(const char *topic, uint8_t *data, uint16_t bLen, uint8_t qos) {
bool Adafruit_MQTT::publish(const char *topic, uint8_t *data, uint16_t bLen, uint8_t qos, bool retain) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concern here

bool Adafruit_MQTT::publish(const char *topic, uint8_t *data, uint16_t bLen, uint8_t qos, bool retain) {
// Construct and send publish packet.
uint16_t len = publishPacket(buffer, topic, data, bLen, qos);
uint16_t len = publishPacket(buffer, topic, data, bLen, qos, retain);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here


// Now you can start generating the packet!
p[0] = MQTT_CTRL_PUBLISH << 4 | qos << 1;
p[0] = MQTT_CTRL_PUBLISH << 4 | qos << 1 | (retain ? 1 : 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@Paulie92
Copy link

Is there any example how to call this function in the code? unfortunately I have no idea how can I use it.

Thank you

@adafruit adafruit deleted a comment from adam-ah Apr 26, 2021
@trickkiste
Copy link

When will this be merged?
Seems all tests have passed and it is backward compatible, so what is the holdup?
The feature is sorely needed!

Please merge!

@ben-willmore
Copy link

I made an up-to-date version of this which overloads the publish and publishPacket functions rather than using optional arguments:

https://github.com/ben-willmore/Adafruit_MQTT_Library/tree/retain-flag

@brentru
Copy link
Member

brentru commented Nov 2, 2022

@ben-willmore Are you interested in checking out a pull request against this library, I'll review it.

@ben-willmore
Copy link

@brentru Sure, have now done so -- #216

@brentru
Copy link
Member

brentru commented Nov 11, 2022

Resolved by #216

@brentru brentru closed this Nov 11, 2022
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.

6 participants