search

FactbirdHQ / mqttrust

MQTT client for embedded devices, written in rust
53 stars 5 forks source link

fix Client<const L> parameter docs #52

Closed andresv closed 2 years ago

andresv commented 2 years ago

L does not actually mean the number of packets. It shows underlying BBQueue buffer size in bytes where FrameProducer is taken.

It would be good to give some tips what minimum required length would be. Like if your max expected MQTT payload is 100 bytes and topic name 20 bytes then L should be at least N bytes. @MathiasKoch do you happen to know what it might be?

MathiasKoch commented 2 years ago

I think this documentation can be even further improved.

The length L is actually the number of bytes of serialized MQTT packets that can be squeezed into the transmit buffer.

As an example, if you send a publish packet at QoS 1 with 20 bytes payload and 30 bytes topic name, the serialized packet length would be determined by https://github.com/BlackbirdHQ/mqttrust/blob/63a3a76e597d9a04337baa86686dd634777f32c1/mqttrust/src/encoding/v4/packet.rs#L88-L105 to be 5 + 2 + 20 + 2 + 30 = 59 bytes. With an L length queue of 200 bytes you would be able to client.send() 200 / 59 = 3 of those publish packets before needing to flush them to the network with eventloop.yield(). Usually you would yield on every pass of your main loop, but there are lots of cases where one would push multiple publish packets into the transmit queue in a single event loop pass.

Not sure if above makes sense?

As an example, I often have multiple subscribes happening rapidly in a row.

andresv commented 2 years ago

Yes makes sense. I try to come up how to add this info to docs.

andresv commented 2 years ago

Added some more info about L.

codecov-commenter commented 2 years ago

Codecov Report

Base: 39.16% // Head: 39.21% // Increases project coverage by +0.05% :tada:

Coverage data is based on head (1bb0d3a) compared to base (3f175ec). Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #52 +/- ## ========================================== + Coverage 39.16% 39.21% +0.05% ========================================== Files 13 13 Lines 1177 1178 +1 Branches 338 337 -1 ========================================== + Hits 461 462 +1 Misses 568 568 Partials 148 148 ``` | [Impacted Files](https://codecov.io/gh/BlackbirdHQ/mqttrust/pull/52?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlackbirdHQ) | Coverage Δ | | |---|---|---| | [mqttrust\_core/src/lib.rs](https://codecov.io/gh/BlackbirdHQ/mqttrust/pull/52/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlackbirdHQ#diff-bXF0dHJ1c3RfY29yZS9zcmMvbGliLnJz) | `32.25% <0.00%> (ø)` | | | [mqttrust\_core/src/eventloop.rs](https://codecov.io/gh/BlackbirdHQ/mqttrust/pull/52/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlackbirdHQ#diff-bXF0dHJ1c3RfY29yZS9zcmMvZXZlbnRsb29wLnJz) | `35.98% <0.00%> (+0.30%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlackbirdHQ). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlackbirdHQ)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

MathiasKoch commented 2 years ago

Thanks