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

Invalid CONNACK packet #41

Closed
cepr opened this issue Oct 19, 2022 · 10 comments
Closed

Invalid CONNACK packet #41

cepr opened this issue Oct 19, 2022 · 10 comments

Comments

@cepr
Copy link

cepr commented Oct 19, 2022

The CONNACK packet generated by TinyMqtt doesn't look right.

TinyMqtt generated this: 20 82 00 00 00
I was expecting: 20 02 xx xx (Assuming MQTT 3.1.1)

This bug prevents Adafruit_MQTT_Library from connecting to this broker. I'm sure other MQTT clients will have the same issue.

@xenoeng
Copy link

xenoeng commented Oct 19, 2022

This might explain why I couldn't connect from either MQTT Explorer on the PC or from MQTT Analyser on my phone. I could connect to the AP fine but had no luck with any client. Thought it was something I was missing.

@cepr
Copy link
Author

cepr commented Oct 19, 2022

I fixed this issue by replacing:

MqttMessage msg(MqttMessage::Type::ConnAck);
msg.add(0);	// Session present (not implemented)
msg.add(0); // Connection accepted
msg.sendTo(this);

to:

write("\x20\x02\x00\x00", 4);

Unfortunately, I am getting more errors now.
I switched to sMQTTBroker which works better for me.

@hsaturn
Copy link
Owner

hsaturn commented Oct 29, 2022

Hello cepr

For some optimisation reasons, the size with TinyMQTT is always on 2 bytes.
This is the reason for which you have 0x82 00 and not 0x02, this should not be a bug but ....

And.... for an obscure reason, the ConnAck packet does not respect the variable packet length encoding (see 2.2.3 Remaining Length).

As you've pointed this bug, I have to rewrite the packet length encoding...

Thanks for your report.

@hsaturn
Copy link
Owner

hsaturn commented Oct 30, 2022

Back...

I wrote a data observer in ESP8266 Wifi mock in order to add a unit test before fixing that bug.

@hsaturn
Copy link
Owner

hsaturn commented Oct 30, 2022

Some news : using the new feature of ESP8266 Mock, I can see this :

       |   data sent  15 : Connect     10 8C 00 00 04 4D 51 54 54 04 00 00 0A 00 00 
       |   data sent   5 : ConnAck     20 82 00 00 00 
       |   data sent  11 : Subscribe   82 88 00 00 00 00 03 61 2F 62 00 
       |   data sent   6 : SubAck      90 83 00 00 00 00 
       |   data sent  11 : Subscribe   82 88 00 00 00 00 03 61 2F 62 00 
       |   data sent   3 : Disconnect  E0 80 00 

Thanks to this unit test, the fix will long last :-)

@hsaturn
Copy link
Owner

hsaturn commented Oct 30, 2022

Hello cepr

I've fixed the length encoding for (all) messages.
It it possible for you to check the latest git version (not released yet) so can release the version ?

       |   data sent  14 : Connect     10 0C 00 04 4D 51 54 54 04 00 00 0A 00 00                   [....MQTT......]
       |   data sent   4 : ConnAck     20 02 00 00                                                 [ ...]
       |   data sent  10 : Subscribe   82 08 00 00 00 03 61 2F 62 00                               [�.....a/b.]
       |   data sent   5 : SubAck      90 03 00 00 00                                              [�....]
       |   data sent  10 : Subscribe   82 08 00 00 00 03 61 2F 62 00                               [�.....a/b.]
       |   data sent   2 : Disconnect  E0 00                                                       [�.]

Thanks again for reporting.

@cepr
Copy link
Author

cepr commented Oct 30, 2022

Thanks hsaturn, I'll try now!

@hsaturn
Copy link
Owner

hsaturn commented Oct 30, 2022

Thanks hsaturn, I'll try now!

Thanks to you. Keep in mind that this is the git version yet, the lib has not been released with the fix yet.

@cepr
Copy link
Author

cepr commented Oct 30, 2022

It works, thanks hsaturn.

Here are the tests I did:
rev 7bd299e

This broker is now compatible with the following clients:

  • NodeRed on Linux (npm module "mqtt", rev 4.3.7, https://www.npmjs.com/package/mqtt)
  • Platform.IO library 256dpi/MQTT@^2.5.0) on ESP8266
  • Platform.IO library adafruit/Adafruit MQTT@^2.4.3 on ESP8266

The board I am using is not very common and requires a different include for the WiFi to work, I added those lines in TinyMqtt.h for my testing:

#ifdef WIO_TERMINAL
#include <rpcWiFi.h>
#endif

@hsaturn
Copy link
Owner

hsaturn commented Oct 30, 2022

Thanks a lot Cedric !!!

I will add the 3 lines you mention. Even if I do not know who else than you will use them, and even if I do not understand what they mean.

I have released the 0.9.0 version with this bugfix. But it seems now that my lib cannot be installed easilly (dependency with AsyncTCP...)

Also I have to fix a bug on incomplete decoding of topics (Issue #30).

Release 0.9.2 includes your lines.
The 1.0.0 is not far, thanks to you

@hsaturn hsaturn closed this as completed Oct 30, 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

No branches or pull requests

3 participants