-
Notifications
You must be signed in to change notification settings - Fork 26
MTM-64963 Rework MQTT Implementation for GA #3925
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
base: mqtt-service-ga-integration
Are you sure you want to change the base?
MTM-64963 Rework MQTT Implementation for GA #3925
Conversation
* Move some text to new connecting devices section * Re-do the first few implementation sub-sections * Move remaining text into the new structure
(still need to understand how Request Problem Information works)
|
Preview available here |
content/device-integration/mqtt-service-bundle/connecting-devices.md
Outdated
Show resolved
Hide resolved
content/device-integration/mqtt-service-bundle/connecting-devices.md
Outdated
Show resolved
Hide resolved
content/device-integration/mqtt-service-bundle/connecting-devices.md
Outdated
Show resolved
Hide resolved
content/device-integration/mqtt-service-bundle/implementation.md
Outdated
Show resolved
Hide resolved
content/device-integration/mqtt-service-bundle/implementation.md
Outdated
Show resolved
Hide resolved
content/device-integration/mqtt-service-bundle/implementation.md
Outdated
Show resolved
Hide resolved
|
|
||
| | Feature | Support level | Notes | | ||
| |------------------------------|---------------|------------------------------------------------------------------------------------------------------------------| | ||
| | Client Identifier | Mandatory | As for [version 3.1.1](#client-id). | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | Client Identifier | Mandatory | As for [version 3.1.1](#client-id). | | |
| | Client identifier | Mandatory | As for [version 3.1.1](#client-id). | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scmi-c8y Please check these tables for unnecessary capitalization. Most of the terms do not need capitalization, except for the first characters which we always capitalize. But I´m not 100% sure which ones are proper names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BeateRixen Good question. I used the same capitalization as the MQTT specification, e.g. https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901046, which seems to treat the names of fields, packet types, etc. as proper names. I agree it doesn't necessarily align well with our style though. I'm happy to follow your guidance on this.
content/device-integration/mqtt-service-bundle/implementation.md
Outdated
Show resolved
Hide resolved
|
|
||
| MQTT _retained messages_ are not supported by the MQTT Service. | ||
| If the retain flag is set on a `PUBLISH` message from a device, the message will not be accepted and the connection will be closed. | ||
| Devcies should not attempt to re-send an unacknowledged QoS 1 retained message after reconnecting, as this will simply cause the connection to be closed again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Devcies should not attempt to re-send an unacknowledged QoS 1 retained message after reconnecting, as this will simply cause the connection to be closed again. | |
| Devices should not attempt to re-send an unacknowledged QoS 1 retained message after reconnecting, as this will simply cause the connection to be closed again. |
On the other hand, I'm not sure if we need this sentence at all - it was initially misleading for me. Why the device may like to re-send retained messages if we don't accept it at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I will just delete this.
| | Receive Maximum | Supported | The MQTT Service will limit the number of unacknowledged QoS 1 messages for the device to the requested maximum. | | ||
| | Maximum Packet Size | Supported | The MQTT Service will not send any message larger than the requested size to this device.<br>Note that messages larger than the reqeusted size will be **silently discarded**. | | ||
| | Request Problem Information | Supported | A device should not assume that the MQTT Service will send a reason string, even when this has been requested. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should double-check if the HiveMQ is supporting those out of the box as they were never tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the maximum packet size - that works as expected. The other two are harder to test. I would strongly expect "receive maximum" to work (it would be a Hive bug if it didn't). As far as I understand it "request problem information" is somewhat optional anyway, so the server doesn't have to send a reason string even if the client requests it. We can probably claim it is supported even if the behavior doesn't change at all.
I'm pretty confident about the first two but could change "request problem information" to "Ignored" if you think that would be better?
| These topic name cannot be used by devices: | ||
|
|
||
| * All _system topics_ (topic name beginning with `$`) unless specifically documented | ||
| * `devicecontrol/notifications` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: In fact, it can be used, but it is only for SmartREST. In my opinion, we should move it to the Core MQTT topics chapter.
| Clients can connect using version 5.0 of the MQTT protocol. | ||
| Support for additional MQTT 5.0 features will be added in future releases. | ||
| There is no overlap between the Core MQTT and generic device topic spaces. | ||
| However, to avoid potentially hard to diagnose failures, generic devices should avoid using any topic name starting with the Core MQTT prefixes `s/`, `t/`, `q/` or `c/`, even though some topics under those prefixes are not used by Core MQTT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: We should probably also list JSON over MQTT topics here. They all start from alarm/alarms/, event/events, measurement/measurements/, inventory/managedObjects/
| See the [Service Quotas](/service-terms/quotas#mqtt-service) section for details of these limits. | ||
| The soft quota can be increased up to the hard limit on request, although this may incur additional costs for the tenant. | ||
| The message size used by these limits includes the message header as well as the payload. | ||
| Message header size can vary significantly, particularly for MQTT version 5.0 devices, but it will always be at least 2 bytes, and usually more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abor-c8y should we add more details here? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this covers it. Although if we want to be more thorough I have found that the HiveMQ interceptors result in the following:
Extra Byte Allowance
MQTT5 Connect (Total of 25 bytes)
The following properties are resolved to mqtt spec defaults by the time interceptor receives the packet:
Variable header:
- 5 bytes for session expiry
- 3 bytes for receive maximum
- 5 bytes for maximum packet size
- 3 bytes for topic alias maximum
- 2 bytes for request response information
- 2 bytes for request problem information
Payload: - 5 bytes for will delay interval
MQTT5 Publish (Total of 8 bytes)
The following properties are resolved to mqtt spec defaults by the time interceptor receives the packet:
Variable header:
- 3 bytes for topic alias
- 5 bytes for message expiry interval (what value do we get when we get this packet)
With the work that I have in review we will have to allow an extra 25 and 8 bytes for MQTT5 connect and publish packets respectively.
| The MQTT Service follows the MQTT specification for responses from the server to devices. | ||
|
|
||
| #### Adding and trusting CA certificate | ||
| According to the specification, if the server receives a malformed packet or a protocol error, it must disconnect the device. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: That's true only for MQTT 3.1.1. MQTT 5.0 usually does not disconnect the client, it just sends back a negative response.
| Once the CA certificate has been uploaded and trusted in {{< product-c8y-iot >}}, devices can authenticate using client certificates signed by your trusted CA. | ||
| To connect using any MQTT client, use the previously generated client certificate and key. | ||
| This example uses the Mosquitto MQTT client: | ||
| <font color="red" size="24">**TBC -- need details of the alarms**</font> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently alarms are raised when processing message from the to-device topic:
c8y_MqttService_ToDevice_NoKey- Pulsar message has no key.c8y_MqttService_ToDevice_InvalidKey- Pulsar message has an unexpected key. For device isolation the key did not correspond to a subscribed clients id, for tenant isolation the message key did not match the topic.c8y_MqttService_ToDevice_EmptyTopic- Message property topic was an empty string.c8y_MqttService_ToDevice_EmptyClientId- Message property clientID was an empty string.
Co-authored-by: Beate Rixen <90445236+BeateRixen@users.noreply.github.com>
Co-authored-by: Beate Rixen <90445236+BeateRixen@users.noreply.github.com> Co-authored-by: Sławomir Chmiel <slawomir.chmiel@cumulocity.com>
I'm not super-happy with this but I've been struggling to spend time on it, so I want to at least get it out there for people to start looking at. The reviewers are chosen because you have worked specifically on the MQTT side of the service, so you should have the knowledge to tell me what I've got wrong. I did find a few issues and questions while I was trying to verify specific behaviour but I will log those separately.
Notes
connecting-devices.mdfile is also a placeholder that will be re-worked in the next PR. You don't need to review this.