removing outbound buffering (and size limits) from mqtt #2

Open
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
@karlp

karlp commented May 23, 2011

We wanted to send slightly bigger messages here at ReMake Electric, so we fixed up the library a little bit. It now uses the pointer given by the user, and copies straight from there to the network, rather than via the internal packet buffer.

This means that all outbound messages are now no longer limited in size by the library, but by the user. MAX_PACKET_SIZE is now only relevant for received packets. (It also makes it faster, but that's not really an issue)

karlp added some commits May 23, 2011

NoCopy writes for all outbound mqtt messages.
No longer hard limited to <128 byte messages.  Outbound messages are now only
limited by the user's RAM size, (they provide the pointer) rather than by an
internal library buffer.

Receive messages are still limited to the library buffer, but changing the
single #define is now sufficient to change the allowed size.

Headers were updated to provide better code assistance in IDEs.

Work by ReMake Electric, happily returned to the community.
Removing the #ifdef option for outbound buffering.
No need to ever use this, so don't keep it around #deffed out.
Use consistent return codes in PubSubClient.
Some methods previously returned 0 for success, (and for failure) and some returned 1 for success and 0 for failure.  Make them all use 0 for success, and different error codes for each failure, and make them proper defines.

This is BACKWARDS INCOMPATIBLE unfortunately :(

Calls that were
   while (client.connect(config.devicename) != 1)
Should now be
   while ((rc = client.connect(config.devicename)) != ERR_OK)

The old code used 1 for success, 0 for failure for:
   connect()
   loop()

The old code used 0 for both success and failure for:
   publish()

The old code used no return code at all for:
   subscribe()

In all cases, the new code returns 0 (ERR_OK) for success, and either
ERR_NOT_CONNECTED or ERR_TIMEOUT_EXCEEDED for the failure cases.  They are now
also broken out per failure code.
@knolleary

This comment has been minimized.

Show comment Hide comment
@knolleary

knolleary Jun 2, 2011

Owner

Hi Karl - just a quick note to say I'm not ignoring this, just snowed under at the moment.

I've noticed you're still adding some changes - including ones that will break the api (the more consistent return codes).
I need to be careful when adding api-breaking changes as there are a number of folk depending on this library.

That said, the forthcoming Arduino-1.0 is an opportunity to make such changes as it introduces changes of its own. I've already identified some changes needed in this library to accommodate the api changes in Arduino-1.0. My plan is to make the necessary changes, merge your changes (once I've reviewed them properly) and do a v1.7 release to coincide with the Arduino-1.0 release - but I won't get a chance to do that for a couple weeks.

Cheers,
Nick

Owner

knolleary commented Jun 2, 2011

Hi Karl - just a quick note to say I'm not ignoring this, just snowed under at the moment.

I've noticed you're still adding some changes - including ones that will break the api (the more consistent return codes).
I need to be careful when adding api-breaking changes as there are a number of folk depending on this library.

That said, the forthcoming Arduino-1.0 is an opportunity to make such changes as it introduces changes of its own. I've already identified some changes needed in this library to accommodate the api changes in Arduino-1.0. My plan is to make the necessary changes, merge your changes (once I've reviewed them properly) and do a v1.7 release to coincide with the Arduino-1.0 release - but I won't get a chance to do that for a couple weeks.

Cheers,
Nick

@karlp

This comment has been minimized.

Show comment Hide comment
@karlp

karlp Jun 3, 2011

oh, hmm, github mutated my pull request. I only wanted the pull request to pull in 4ec2ef9, 7dfb29e, bfa4590. The return code changes are API changes, and warrant separate discussion. I guess I should have had a separate branch for the pull request so I could keep working on my own master?

karlp commented Jun 3, 2011

oh, hmm, github mutated my pull request. I only wanted the pull request to pull in 4ec2ef9, 7dfb29e, bfa4590. The return code changes are API changes, and warrant separate discussion. I guess I should have had a separate branch for the pull request so I could keep working on my own master?

@karlp

This comment has been minimized.

Show comment Hide comment
@karlp

karlp Jun 24, 2011

One thing I should note, is that by removing the buffering in the PubSubClient, my changes here make the overhead due to http://code.google.com/p/arduino/issues/detail?id=563 substantially worse. I'm preparing a patch for Arduino core's TCP client, and subsequent patch to PubSubClient to make use of said patch, but I don't expect Arduino to accept my patch anytime soon.

This is not a problem at all if you're operating on a LAN, it's not very efficient, but it's not really much of a problem. On a WAN however, you will be using substantially more traffic than you expected due to the TCP/IP overhead. (for ~80-100meg of MQTT payload/month, you will be using ~800meg or so of total traffic)

We only noticed this here recently :(

karlp commented Jun 24, 2011

One thing I should note, is that by removing the buffering in the PubSubClient, my changes here make the overhead due to http://code.google.com/p/arduino/issues/detail?id=563 substantially worse. I'm preparing a patch for Arduino core's TCP client, and subsequent patch to PubSubClient to make use of said patch, but I don't expect Arduino to accept my patch anytime soon.

This is not a problem at all if you're operating on a LAN, it's not very efficient, but it's not really much of a problem. On a WAN however, you will be using substantially more traffic than you expected due to the TCP/IP overhead. (for ~80-100meg of MQTT payload/month, you will be using ~800meg or so of total traffic)

We only noticed this here recently :(

@karlp

This comment has been minimized.

Show comment Hide comment
@karlp

karlp Jun 24, 2011

So, if anyone wants to use this, without having to wait for Arduino upgrades and so on: https://github.com/karlp/pubsubclient/tree/efficient_tcp has the patches to PubSubClient to use the proposed arduino patches.

karlp commented Jun 24, 2011

So, if anyone wants to use this, without having to wait for Arduino upgrades and so on: https://github.com/karlp/pubsubclient/tree/efficient_tcp has the patches to PubSubClient to use the proposed arduino patches.

@knolleary

This comment has been minimized.

Show comment Hide comment
@knolleary

knolleary Jan 12, 2013

Owner

Since this was original raised, a number of changes have gone into the library that address a lot of what you cover in this issue.

The main difference is the library still uses its internal buffer - rather than user provided RAM - but it does minimise the network writes (to the extent the underlying library minimises them) and uses consistent return codes across the functions.

I'll have a think about the outbound buffer - it would remove the need to copy in to the buffer; but it wouldn't be possible to pass the whole packet to the network in a single call to write.

Owner

knolleary commented Jan 12, 2013

Since this was original raised, a number of changes have gone into the library that address a lot of what you cover in this issue.

The main difference is the library still uses its internal buffer - rather than user provided RAM - but it does minimise the network writes (to the extent the underlying library minimises them) and uses consistent return codes across the functions.

I'll have a think about the outbound buffer - it would remove the need to copy in to the buffer; but it wouldn't be possible to pass the whole packet to the network in a single call to write.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment