feat(mqtt): add Mqtt5ConnectionPropertyConfig — expose MQTT 5.0 CONNECT properties via MqttClientConfiguration - #659
Conversation
979b1c2 to
fcf8f1d
Compare
|
@daniil4udo Looking at the PR details, it seems to contribute MQTT 5.0 support to the However, rather than having a description along those lines, the current description of this PR contains some LLM gibberish which seems to be the last thing it did produce while implementing the MQTT 5.0 support. Therefore, could you please restore the original PR template (the one available when opening a new PR) and try to fill it in (or let your LLM fill it in). This template is in there for a good reason. Amongst other things, it has a set of check-points you need to do for your PR to be ready for merge. |
Adds Mqtt5ConnectionPropertyConfig config struct and a mqtt5_connection_property field on MqttClientConfiguration. Internally, EspMqttClient::new_raw calls esp_mqtt5_client_set_connect_property between esp_mqtt_client_init and esp_mqtt_client_start, eliminating the deadlock where calling the setter after start blocks the caller on MQTT_API_LOCK held by mqtt_task across esp_transport_connect() (multi-second TLS handshake). The struct and field names mirror the C binding (esp_mqtt5_connection_property_config_t) so reviewers can grep both sides and see the parity at a glance, matching the existing convention of nested config sub-structs in this file (e.g. LwtConfiguration). Backward-compatible: EspMqttClient::new/new_cb/new_nonstatic_cb signatures are unchanged. Users who do not set mqtt5_connection_property are unaffected. Also adds MqttProtocolVersion::V5 enum variant (gated on esp_idf_mqtt_protocol_5) required to negotiate MQTT 5.0 on the wire — the C setter returns ESP_FAIL if the client was not initialized with protocol_ver = MQTT_PROTOCOL_V_5.
…r on Mqtt5ConnectionPropertyConfig Completes scalar-field coverage of esp_mqtt5_connection_property_config_t. The two new fields apply to the Last Will message (LWT) and are only meaningful when MqttClientConfiguration::lwt is also set. Remaining unexposed fields are pointer-typed (content_type, response_topic, correlation_data, user_property, will_user_property) and require additional lifetime/borrowing design — deferred to a follow-up.
- Add cross-reference doc to EspMqttClient::new pointing to
mqtt5_connection_property and the V5 protocol version requirement
- Correct maximum_packet_size: None docs (clamps to RX buffer, not unlimited)
- Document request_response_info / request_problem_info Some(false) C-boundary
limitation (ESP-IDF encoder skips bool properties with value false; both None
and Some(false) are indistinguishable at the wire level)
- Add 'C binding field: request_resp_info' note on request_response_info
- Replace unsafe { core::mem::zeroed() } with Default::default() (bindgen
generates a safe Default impl that zero-initialises the struct identically)
… in CI Adding V5 without #[non_exhaustive] is a breaking change for downstream callers with exhaustive match arms on MqttProtocolVersion. The attribute lets future variants be added without a semver bump and requires callers to use a wildcard arm, which is correct for a protocol-version enum. Also adds CONFIG_MQTT_PROTOCOL_5=y to .github/configs/sdkconfig.defaults so the new cfg(esp_idf_mqtt_protocol_5)-gated code is compiled and clippy-checked in all four CI workflow jobs (ci.yml, ci-esp-idf-next.yml, publish.yml, publish-dry-run.yml). Previously the new struct, field, and new_raw setter block were silently skipped in every build.
- Add #[non_exhaustive] to Mqtt5ConnectionPropertyConfig (parallel to MqttProtocolVersion) so future scalar fields can be added without a semver break for callers using ..Default::default() - Add pre-flight Rust check: if mqtt5_connection_property is Some but protocol_version != Some(MqttProtocolVersion::V5), return Err(ESP_ERR_INVALID_ARG) before the FFI call, replacing the opaque ESP_FAIL(259) that the C setter would have returned - Document Some(0) = None C-boundary limitation for session_expiry_interval, will_delay_interval, receive_maximum, topic_alias_maximum (ESP-IDF encoder skips zero-valued numeric properties; explicit zero is indistinguishable from the protocol default) - Revert CONFIG_MQTT_PROTOCOL_5=y from .github/configs/sdkconfig.defaults: enabling MQTT5 is downstream's responsibility; the library CI should not impose it on all builds
#[non_exhaustive] prevents struct literal construction from outside the defining crate (E0639), which defeats the purpose of a user-facing config struct. It is correct on MqttProtocolVersion (enum variants) but wrong here. Forward-compat for the struct is achieved by the idiomatic ..Default::default() pattern, which callers already use.
0ece158 to
a566d28
Compare
Maintainer feedback: the prior comments and changelog wording read as LLM-generated noise. Tighten to the file's existing terse style: - Drop multi-line doc paragraphs on MqttProtocolVersion::V5, Mqtt5ConnectionPropertyConfig, its fields, and the new config field - Drop the per-field commentary; field names are self-describing - Replace nine if-let assignment statements with a single struct literal using Option::unwrap_or and ..Default::default() - Drop the long SAFETY block; one short comment explains the init/start ordering constraint - Collapse the two MQTT 5.0 CHANGELOG entries to single lines
- Replace `unwrap_or(0/false)` ladder with conditional assignment from `Default::default()`. `None` no longer silently becomes `0`/`false` at the C boundary; unset fields stay at the C struct's zero default, which the ESP-IDF encoder skips so the broker applies the MQTT 5 protocol default. Avoids surprises on fields whose protocol default differs from zero (e.g. Request Problem Information defaults to 1) - Log an explanatory `error!` line before returning `ESP_ERR_INVALID_ARG` when `mqtt5_connection_property` is set without `protocol_version = Some(V5)`, so the misconfiguration is discoverable without grepping ESP-IDF error codes - Restore minimal per-field doc comments giving units and the MQTT 5 spec section, plus a struct-level note that fields may be added in minor releases (callers should use `..Default::default()`) - Enable `CONFIG_MQTT_PROTOCOL_5=y` in CI sdkconfig.defaults so the new `#[cfg(esp_idf_mqtt_protocol_5)]` paths are compile-checked - Add a CONNECT-properties usage snippet to the CHANGELOG entry
|
@ivmarkov thank you for the feedback, fix it. |
|
@copilot resolve the merge conflicts in this pull request |
Right, I completely managed to forget about this PR :-( Anyways. I think it is genuinely useful. what it is missing a bit is the MQTT5's support for user-define connection properties, but I guess even without it, it is good for merging. Thanks a lot! |
Submission Checklist 📝
cargo fmtcommand to ensure that all changed code is formatted correctly.cargo clippycommand to ensure that all changed code passes latest Clippy nightly lints.CHANGELOG.mdin the proper section.Pull Request Details 📖
Adds MQTT 5.0 CONNECT-property support to
EspMqttClient, the type-safe Rust wrapper around the ESP-IDF MQTT component.The underlying C function
esp_mqtt5_client_set_connect_property()cannot safely be called from user code afterEspMqttClient::new()returns: by thenmqtt_taskis running and holdsMQTT_API_LOCKacrossesp_transport_connect(), so the call deadlocks. The only safe window is betweenesp_mqtt_client_ini()andesp_mqtt_client_start(), which the wrapper now does internally.Description
MqttProtocolVersion::V5(gated onCONFIG_MQTT_PROTOCOL_5=y), required to negotiate MQTT 5.0 on the wire.MqttProtocolVersionis marked#[non_exhaustive]so future variants do not break downstream match arms.Mqtt5ConnectionPropertyConfig, exposing the CONNECT-time properties:session_expiry_interval,will_delay_interval,receive_maximum,maximum_packet_size,topic_alias_maximum,request_response_info,request_problem_info,message_expiry_interval,payload_format_indicator. Each field isOption<T>; None leaves the property unset so the broker applies the MQTT 5 protocol default (the ESP-IDF encoder skips zero-valued fields).Err(ESP_ERR_INVALID_ARG)(with anerror!log line for discoverability) ifmqtt5_connection_propertyis set withoutprotocol_version: Some(MqttProtocolVersion::V5), replacing the opaqueESP_FAILthe C setter would otherwise return.CONFIG_MQTT_PROTOCOL_5=yin the CIsdkconfig.defaultsso the new#[cfg(esp_idf_mqtt_protocol_5)]paths are compile-checked across the build matrix.All new types and fields are gated on
#[cfg(esp_idf_mqtt_protocol_5)], so downstream crates that don't enable MQTT 5 in theirown sdkconfig see no API surface change. The
MqttProtocolVersionenum gets#[non_exhaustive]as a one-time defensive addition;the existing
V3_1andV3_1_1variants are unchanged on the wire.Usage:
Testing
Tested on ESP32-S3 hardware:
CONFIG_MQTT_PROTOCOL_5=yandprotocol_version: Some(MqttProtocolVersion::V5), setsession_expiry_intervalandreceive_maximum, confirmed via broker logs that the values appear in the CONNECT packet.mqtt5_connection_propertywithoutMqttProtocolVersion::V5returnsESP_ERR_INVALID_ARG(with a log line) instead of hanging onMQTT_API_LOCKor returning opaqueESP_FAIL.