MQTTv5 Packet bug fixes and samples

eclipse-paho / paho.mqtt.embedded-c

Paho MQTT C client library for embedded systems. Paho is an Eclipse IoT project (https://iot.eclipse.org/)

https://eclipse.org/paho

Other

1.37k stars 757 forks source link

MQTTv5 Packet bug fixes and samples #241

Closed CIPop closed 1 year ago

CIPop commented 1 year ago

Adding new MQTTV5Packet samples (derived from existing v3 samples). The new samples showcase topic aliasing, user properties and *ACK properties.
BugFix: User Properties bug fix (picked commit from #234, maintained commit user assignment /cc @MrEthan). Includes Unit Test changes.
BugFix: User Properties bug fix to exclude NULL characters [MQTT-1.5.4-2] from the MQTTv5 OASIS specification. Includes Unit Test changes.
BugFix: MQTTv5 reason code should not be extended as a negative int. Includes Unit Test changes.
Samples enhancement: string representation of MQTTv5 reason codes and properties.
Other small changes:
- Excluding vscode, doxygen generated files from gitignore.
- CMake build of the Linux MQTT Packet Samples.
- Switched all samples to use the publicly available test.mosquitto.org MQTTv3/v5 broker.
- Added instructions for running the tests locally.

CIPop commented 1 year ago

@icraggs I have verified that the original PR#234 does have the ECA requirement. Merging the PRs in order should automatically resolve any conflicts. If not, I'll be rebasing mine once #234 is merged.

CIPop commented 1 year ago

Thanks for the guidance @icraggs! I will make the modifications by EOD today.

Since it is acceptable, I will be removing my name from all file headers: I was complying with project patterns, but my company prefers scrubbing developer names from source code.

icraggs commented 1 year ago

Thanks for the guidance @icraggs! I will make the modifications by EOD today.

Since it is acceptable, I will be removing my name from all file headers: I was complying with project patterns, but my company prefers scrubbing developer names from source code.

When I was working for IBM, IBM held the copyright, which is why their name is on the headers. Now I work for myself, so I can use my own name :-) It's whatever your company prefers.

CIPop commented 1 year ago

@icraggs I've updated the copyright notices - PTAL to ensure this is indeed the correct form.

icraggs commented 1 year ago

I opened a GitLab issue which according to the IP policy we are obliged to do for PRs over 1000 lines (https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/8715 - I'm waiting for a response). It will be easier in future if a PR is split into smaller ones. This restriction does not apply to committers.

I've also set up a GitHub action to run the Linux CI tests. You can see the results for this PR in the checks above.

CIPop commented 1 year ago

Thank you @icraggs ! I will split up PRs in the future. We were aware and did expect the additional review (this has been discussed in the last IoT WG meeting as well).

I will be out on vacation for 2 weeks: I will continue to monitor the issue if changes are required, but there will be some delays.