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

pkg/paho-mqtt: Update doc.txt #21136

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

DeJusten
Copy link

Add some Information about the paho stack in the doc.txt file

Contribution description

  • Add some Information about the MQTT Version und the runnin behavior

Testing procedure

  • Only documentation, no testing necessary

Issues/PRs references

Add some Information about the paho stack
@github-actions github-actions bot added Area: network Area: Networking Area: doc Area: Documentation Area: pkg Area: External package ports labels Jan 14, 2025
@benpicco benpicco changed the title Update doc.txt pkg/paho-mqtt: Update doc.txt Jan 14, 2025
@benpicco benpicco requested a review from OlegHahm January 14, 2025 18:12
@see https://github.com/eclipse/paho.mqtt.embedded-c

The Eclipse Paho project provides open-source client
implementations of MQTT.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that redundant to the @brief text?

Copy link
Author

Choose a reason for hiding this comment

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

It is duplicated. However, I only removed the asterisk here. The rest is original

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd still remove the duplicated sentence here.

pkg/paho-mqtt/doc.txt Outdated Show resolved Hide resolved
pkg/paho-mqtt/doc.txt Outdated Show resolved Hide resolved
@OlegHahm
Copy link
Member

Thanks for the contribution and improvement - and welcome to RIOT!

And sorry for my somewhat pedantic review: actually I thought to review a PR my a fellow RIOT maintainer. 😅

@MrKevinWeiss
Copy link
Contributor

ping @DeJusten I think some simple fixes are needed to get in.

DeJusten and others added 2 commits January 30, 2025 09:49
Thanks for correcting. Next time I will pay attention to correct pronunciation

Co-authored-by: Oleg Hahm <[email protected]>
Also thanks for the correction

Co-authored-by: Oleg Hahm <[email protected]>
Comment on lines 13 to 14
- MQTT-Version = 3.1 (MQTTVersion struct member of MQTTPacket_connectData = 3)
- MQTT-Version = 3.1.1 (MQTTVersion struct member of MQTTPacket_connectData = 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

So two different MQTT versions are supported? Then I would prefer plural in the sentence above.

Copy link
Author

Choose a reason for hiding this comment

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

That's correct. But I don't know how to change this in this stage.

Copy link
Contributor

Choose a reason for hiding this comment

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

please see #21176 (comment)

- MQTT-Version = 3.1 (MQTTVersion struct member of MQTTPacket_connectData = 3)
- MQTT-Version = 3.1.1 (MQTTVersion struct member of MQTTPacket_connectData = 4)

# Implementation specific information
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Implementation specific information
# Implementation-specific information


# Implementation specific information

- The main loop is running in a dedicated thread (` paho_mqtt_riot`) in the function `mqtt_riot_run()` with a default-cyclus of 30ms (MQTT_YIELD_POLLING_MS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- The main loop is running in a dedicated thread (` paho_mqtt_riot`) in the function `mqtt_riot_run()` with a default-cyclus of 30ms (MQTT_YIELD_POLLING_MS)
- The main loop is running in a dedicated thread (`paho_mqtt_riot`) in the function `mqtt_riot_run()` with a default cycle of 30ms (MQTT_YIELD_POLLING_MS)

can these defaults be changed? If so, could you briefly state how?

# Implementation specific information

- The main loop is running in a dedicated thread (` paho_mqtt_riot`) in the function `mqtt_riot_run()` with a default-cyclus of 30ms (MQTT_YIELD_POLLING_MS)
and an default duration of 10ms (PAHO_MQTT_YIELD_MS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
and an default duration of 10ms (PAHO_MQTT_YIELD_MS)
and a default duration of 10ms (PAHO_MQTT_YIELD_MS)

Also I'm not sure I understand what you mean by cycle and duration here. Are those special MQTT terms?

- The main loop is running in a dedicated thread (` paho_mqtt_riot`) in the function `mqtt_riot_run()` with a default-cyclus of 30ms (MQTT_YIELD_POLLING_MS)
and an default duration of 10ms (PAHO_MQTT_YIELD_MS)
- When publishing a topic with `MQTTPublish()` and QoS>0, the functions blocks until
it one acknowledgement (QoS=1) or two acknowledgements (QoS=2) are received..
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it one acknowledgement (QoS=1) or two acknowledgements (QoS=2) are received..
one (QoS=1) or two acknowledgements (QoS=2) are received.

Comment on lines -2 to +5
* @defgroup pkg_paho_mqtt PAHO MQTT framework
* @ingroup pkg
* @brief The Eclipse Paho project provides open-source client implementations of MQTT for embedded systems
* @see https://github.com/eclipse/paho.mqtt.embedded-c
*
* The Eclipse Paho project provides open-source client
* implementations of MQTT.
@defgroup pkg_paho_mqtt PAHO MQTT framework
@ingroup pkg
@brief The Eclipse Paho project provides open-source client implementations of MQTT for embedded systems
@see https://github.com/eclipse/paho.mqtt.embedded-c
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure whether the stars are needed. Let's see what CI makes out of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 7, 2025
@riot-ci
Copy link

riot-ci commented Feb 7, 2025

Murdock results

✔️ PASSED

9b75770 Correction of the documentation according to the suggestions

Success Failures Total Runtime
1 0 1 02m:16s

Artifacts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation Area: network Area: Networking Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants