hsaturn / TinyMqtt

ESP 8266 / 32 / WROOM Small footprint Mqtt Broker and Client
GNU General Public License v3.0
198 stars 41 forks source link

Message ID #77

Closed real-bombinho closed 1 year ago

real-bombinho commented 1 year ago

Do you capture the message ID of transmitted messages? I could not find anything hinting on on a message ID, so I did a quick and dirty function to return the ID of a message.

in TinyMqtt.h I have added to class MqttMessage:

const char* PublishID() const { return &buffer[(static_cast(buffer[3]) << 8) + static_cast(buffer[4]) + 5];}

in TinyMqtt.cpp I have added to void MqttClient::processMessage(MqttMessage* mesg) inside

case MqttMessage::Type::Publish:

uint8_t qos = (mesg->flags() / 2) && 3;
if (qos == 1) 
        { 
          auto ID = mesg->PublishID();
          MqttMessage msg(MqttMessage::Type::PubAck);
          msg.add(ID[0]);  // MessageID high
          msg.add(ID[1]);  // MessageID low
          msg.sendTo(this);
        }        
hsaturn commented 1 year ago

Why do you need this ? Messages are processed when they arrive. I had no need to burn memory for storing an ID. I don't see any use case where you want to obtain the ID.

real-bombinho commented 1 year ago

I do have a problem with a client (3rd party beta product) flooding the broker with duplicates on QOS1, responding with PubAck solves the problem and keeps the network less choked.

hsaturn commented 1 year ago

Qos is not and won't be handled. This is too memory consuming for a TinyLibrary.

real-bombinho commented 1 year ago

From what I see QOS 1 is simple, you respond with SubAck and that is it.

hsaturn commented 1 year ago

Could you write a scenario with mosquitto for example. I'm not sure, but I may reject Qos=1. I have to read the spec before around Qos=1. Maybe aso I could patch as you did. But I cannot spend time on this so if you provide me enough information, this may be usefull (maybe mandatory) to ack.

real-bombinho commented 1 year ago

The only other thing I am looking at right now is to resend to subscribers on QOS 0 rather passing 0 or 1 through yet still ignoring SubAck. This should keep the aim of supporting only QOS 0 on the right track and as far as I am aware it is absolutely in the right of the broker to resubmit at a different level than the subscriber subscribes at.

As I am now working of the main branch (thank you for patching) I can simply submit my changes and you look at it and either accept or drop as you are pleased?

hsaturn commented 1 year ago

Hmm. As I said, I have to read the spec, I guess there is something to do to increase TinyMqtt scope to what you are pointing. Esecpially if modifications are light.

Yet, I have to fix more important issues. I'm leaving this issue open. Feel free to add links to doc or every information that can help.

real-bombinho commented 1 year ago

I would not expect to increase the scope. Your aim is to not offer QOS. Hence making all outgoing publishes QOS 0 should be the better way rather passing through QOS 1. This sorts most issues with QOS without getting involved. The effort is to delete one bit in vheader on incoming (as you correctly disconnect and ignore on QOS 2, otherwise 2 bits).

It also solves the problem of subscribing clients getting messages with QOS 1 despite having subscribed at QOS 0, which is a violation in itself.

But since you pass through QOS 1 this generates a horrendous amount of additional traffic by continual resubmitting from clients using QOS 1, which can be eliminated by just responding a PubAck on reception. No memory use, the ID is contained in buffer and just needs extraction. Specification says you must respond with a PubAck.

"If a Server implementation does not authorize a PUBLISH to be performed by a Client; it has no way of informing that Client. It MUST either make a positive acknowledgement, according to the normal QoS rules, or close the Network Connection" [MQTT-3.3.5-2]

The submitting client does not know the QOS subscription level of the subscribing client. And therefore QOS 1 would (and should in accordance to your specification of ignoring QOS) only affect submissions to the broker.

real-bombinho commented 1 year ago

I have submitted the SubAck.

I also have the code ready for dropping the QOS to 0, if you want to have a look at it. It is sitting readily in my DropQOS branch.

hsaturn commented 1 year ago

Hi, thanks for this report.

I agree with these modifications. Is it possible to write unit tests before adding these (unit test can be ran under Linux by cloning AUnit EpoxyDuino and EspMock). Without the unit test, further modifications have a lot of chance to remove this enhancement (as I do not check for Qos != 0 in my projects).

Best regards

real-bombinho commented 1 year ago

I will have a look at it, but cannot promise much attention till mid of May, life is just about to get very hectic here.

real-bombinho commented 1 year ago

I will close this issue in the next days as the issue itself is resolved.

I will open the dropQOS in a separate issue, once I have enough time to look at it. My DropQOS works fine with most clients but fails to disconnection on fragmented messages.

real-bombinho commented 1 year ago

Closed as issue is resolved.