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

Make MQTT_POLL_READ_TIMEOUT_MS configurable through menuconfig/sdkconfig (IDFGH-9196) #245

Closed
ItayDumayVayyar opened this issue Jan 20, 2023 · 2 comments

Comments

@ItayDumayVayyar
Copy link

ItayDumayVayyar commented Jan 20, 2023

Hey, I have implemented my own MQTT outbox for storing persistent messages in NVS. But I encountered a problem.
In esp_mqtt_task() the main logic is to ask the outbox for an item, transmit it, and then poll read for a potential ACK.

esp-mqtt/mqtt_client.c

Lines 1562 to 1568 in bb9c8af

MQTT_API_UNLOCK(client);
if (MQTT_STATE_CONNECTED == client->state) {
if (esp_transport_poll_read(client->transport, MQTT_POLL_READ_TIMEOUT_MS) < 0) {
ESP_LOGE(TAG, "Poll read error: %d, aborting connection", errno);
esp_mqtt_abort_connection(client);
}
}

It appears that esp_transport_poll_read() is utilized with a fixed value for MQTT_POLL_READ_TIMEOUT_MS, currently set at 1 second. This means that using esp_mqtt_client_enqueue() would result in a slow message rate due to the dependency on the timeout period or receipt of an ACK before requesting the next message from the outbox.

@github-actions github-actions bot changed the title Make MQTT_POLL_READ_TIMEOUT_MS configurable through menuconfig/sdkconfig Make MQTT_POLL_READ_TIMEOUT_MS configurable through menuconfig/sdkconfig (IDFGH-9196) Jan 20, 2023
@euripedesrocha
Copy link
Collaborator

Hi @ItayDumayVayyar, thanks for the suggestion. I'm working on this feature, it should be available soon.

@ItayDumayVayyar
Copy link
Author

Hi @ItayDumayVayyar, thanks for the suggestion. I'm working on this feature, it should be available soon.

Great, thanks! Can you estimate in what ESP-IDF version this change will be included?

espressif-bot pushed a commit to espressif/esp-idf that referenced this issue Sep 4, 2023
git log --oneline dffabb067fb3c39f486033d2e47eb4b1416f0c82..301bd9e0282eb993523b84d6c56d2559346aa071

Detailed description of the changes:
* fix: Added missing update to message data
  - See merge request espressif/esp-mqtt!189
  - See commit espressif/esp-mqtt@cc41d1b
* PR: fixing typos in `mqtt5_error_reason_code`
  - See merge request espressif/esp-mqtt!188
  - feat: Add enum definition with typo to keep backwards compatibility (espressif/esp-mqtt@90b4a45)
  - fixing typos in `mqtt5_error_reason_code` (espressif/esp-mqtt@dc775bb)
* docs: Clarify keepalive timeout
  - See merge request espressif/esp-mqtt!186
  - See commit espressif/esp-mqtt@cb1e6cf
* fix: LOG format strings
  - See merge request espressif/esp-mqtt!187
  - See commit espressif/esp-mqtt@a3b04f2
* Fix: Stop client only if it's running.
  - See merge request espressif/esp-mqtt!183
  - See commit espressif/esp-mqtt@36eec6f
* fix: Error on publish message creation was ignored.
  - See merge request espressif/esp-mqtt!185
  - See commit espressif/esp-mqtt@585e3ba
* Fix: Allocation for connection buffer was incorrectly done.
  - See merge request espressif/esp-mqtt!182
  - See commit espressif/esp-mqtt@6c849c6
* Adds mqtt host tests to Ci
  - See merge request espressif/esp-mqtt!181
  - See commit espressif/esp-mqtt@4050df4
* Fix: Outbox was leaked in case of initialization failure
  - See merge request espressif/esp-mqtt!180
  - See commit espressif/esp-mqtt@5d491a4
* feat: Add option to bind interface of use
  - See merge request espressif/esp-mqtt!179
  - Closes espressif/esp-mqtt#253
  - See commit espressif/esp-mqtt@363fbf7
* Add outbox size control feature
  - See merge request espressif/esp-mqtt!141
  - feat: Introduces outbox limit (espressif/esp-mqtt@372ab7b)
  - Removes unused outbox functions. (espressif/esp-mqtt@21a5491)
  - refactor: Group access to output buffer in mqtt_connection_t (espressif/esp-mqtt@122875b)
* Add custom transport configuration
  - See merge request espressif/esp-mqtt!169
  - feat: Add custom transport configuration (espressif/esp-mqtt@a5c1b44)
* Removes leftover calls to event_handler
  - See merge request espressif/esp-mqtt!178
  - See commit espressif/esp-mqtt@a492935
* PR: Added support to set server common name.
  - See merge request espressif/esp-mqtt!173
  - Added support to set server common name. (espressif/esp-mqtt@6195762)
* Merge branch 'bugfix/return_on_qos0_disconnected' into 'master'
  - See merge request espressif/esp-mqtt!175
  - See commit espressif/esp-mqtt@5bd9724
* Merge branch 'bugfix/cpp_compilation' into 'master'
  - See merge request espressif/esp-mqtt!174
  - See commit espressif/esp-mqtt@70cbaca
* bug: Incorrect return on disconnect qos0 publish
  - See merge request espressif/esp-mqtt!172
* Fix: Compilation in C++ with multiple subscribe
  - See merge request espressif/esp-mqtt!171
* ci: Fix qemu build against 5.1
  - See merge request espressif/esp-mqtt!170
  - See commit espressif/esp-mqtt@94defb8
* Minor cleanups on mqtt client
  - See merge request espressif/esp-mqtt!168
  - Removes pending message count (espressif/esp-mqtt@da6d38a)
  - Bugfix: Dispatch transport error on all write operations (espressif/esp-mqtt@5729048)
  - Merge enqueue functions (espressif/esp-mqtt@72833c7)
* Adds a configuration for outbox data destination
  - See merge request espressif/esp-mqtt!166
  - feat: Adds a configuration for outbox data destination (espressif/esp-mqtt@2c71f9e)
* CI: Add configuration for ttfw
  - See merge request espressif/esp-mqtt!167
  - See commit espressif/esp-mqtt@d71dcf3
* mqtt5: Fix flow control will increase count when send fragmented packet
  - See merge request espressif/esp-mqtt!164
  - Closes espressif/esp-mqtt#255
  - See commit espressif/esp-mqtt@5cce2c4
* Add license information to queue
  - See merge request espressif/esp-mqtt!163
  - See commit espressif/esp-mqtt@36f0faa
* MQTTv5: Fixes and additions from GitHub PRs
  - See merge request espressif/esp-mqtt!162
  - Merges espressif/esp-mqtt#250
  - fix: Allow MQTT v5 zero length payload (espressif/esp-mqtt@65a4fda)
  - feature: Include subscribe_id in esp_mqtt5_event_property_t (espressif/esp-mqtt@1011e63)
* Feature:  Enable SUBSCRIBE to multiple topics
  - See merge request espressif/esp-mqtt!156
  - See commit espressif/esp-mqtt@3210255
* Adds Kconfig option to configure poll read timeout
  - See merge request espressif/esp-mqtt!159
  - Closes: espressif/esp-mqtt#245
  - See commit espressif/esp-mqtt@2fa945d
* Fix formatting when using printf nano
  - See merge request espressif/esp-mqtt!160
  - See commit espressif/esp-mqtt@e9b865e
* mqtt5: Fix flow control will regard the DUP packet and not consider PUBCOMP packet
  - See merge request espressif/esp-mqtt!158
  - Closes espressif/esp-mqtt#243
  - See commit espressif/esp-mqtt@ed76036
* Remove possible null pointer dereferences
  - See merge request espressif/esp-mqtt!157
  - Bugfix: Remove Remove possible null pointer dereferences (espressif/esp-mqtt@f80772b)
espressif-bot pushed a commit to espressif/esp-idf that referenced this issue Sep 18, 2023
* Update submodule:
git log --oneline dffabb067fb3c39f486033d2e47eb4b1416f0c82..05b347643f6679cc60a50a9664b86a24ebf3ae10

Detailed description of the changes:
* Fix: Mock test should include idf_additions.h
  - See merge request espressif/esp-mqtt!191
  - fix: Mock tests include idf_additions.h (espressif/esp-mqtt@f35aaa1)
* Adds missing documentation to outbox configuration.
  - See merge request espressif/esp-mqtt!190
  - docs: Adds missing documentation to outbox configuration. (espressif/esp-mqtt@c355e0b)
* fix: Added missing update to message data
  - See merge request espressif/esp-mqtt!189
  - See commit espressif/esp-mqtt@cc41d1b
* PR: fixing typos in `mqtt5_error_reason_code`
  - See merge request espressif/esp-mqtt!188
  - feat: Add enum definition with typo to keep backwards compatibility (espressif/esp-mqtt@90b4a45)
  - fixing typos in `mqtt5_error_reason_code` (espressif/esp-mqtt@dc775bb)
* docs: Clarify keepalive timeout
  - See merge request espressif/esp-mqtt!186
  - See commit espressif/esp-mqtt@cb1e6cf
* fix: LOG format strings
  - See merge request espressif/esp-mqtt!187
  - See commit espressif/esp-mqtt@a3b04f2
* Fix: Stop client only if it's running.
  - See merge request espressif/esp-mqtt!183
  - See commit espressif/esp-mqtt@36eec6f
* fix: Error on publish message creation was ignored.
  - See merge request espressif/esp-mqtt!185
  - See commit espressif/esp-mqtt@585e3ba
* Fix: Allocation for connection buffer was incorrectly done.
  - See merge request espressif/esp-mqtt!182
  - See commit espressif/esp-mqtt@6c849c6
* Adds mqtt host tests to Ci
  - See merge request espressif/esp-mqtt!181
  - See commit espressif/esp-mqtt@4050df4
* Fix: Outbox was leaked in case of initialization failure
  - See merge request espressif/esp-mqtt!180
  - See commit espressif/esp-mqtt@5d491a4
* feat: Add option to bind interface of use
  - See merge request espressif/esp-mqtt!179
  - Closes espressif/esp-mqtt#253
  - See commit espressif/esp-mqtt@363fbf7
* Add outbox size control feature
  - See merge request espressif/esp-mqtt!141
  - feat: Introduces outbox limit (espressif/esp-mqtt@372ab7b)
  - Removes unused outbox functions. (espressif/esp-mqtt@21a5491)
  - refactor: Group access to output buffer in mqtt_connection_t (espressif/esp-mqtt@122875b)
* Add custom transport configuration
  - See merge request espressif/esp-mqtt!169
  - feat: Add custom transport configuration (espressif/esp-mqtt@a5c1b44)
* Removes leftover calls to event_handler
  - See merge request espressif/esp-mqtt!178
  - See commit espressif/esp-mqtt@a492935
* PR: Added support to set server common name.
  - See merge request espressif/esp-mqtt!173
  - Added support to set server common name. (espressif/esp-mqtt@6195762)
* Merge branch 'bugfix/return_on_qos0_disconnected' into 'master'
  - See merge request espressif/esp-mqtt!175
  - See commit espressif/esp-mqtt@5bd9724
* Merge branch 'bugfix/cpp_compilation' into 'master'
  - See merge request espressif/esp-mqtt!174
  - See commit espressif/esp-mqtt@70cbaca
* bug: Incorrect return on disconnect qos0 publish
  - See merge request espressif/esp-mqtt!172
* Fix: Compilation in C++ with multiple subscribe
  - See merge request espressif/esp-mqtt!171
* ci: Fix qemu build against 5.1
  - See merge request espressif/esp-mqtt!170
  - See commit espressif/esp-mqtt@94defb8
* Minor cleanups on mqtt client
  - See merge request espressif/esp-mqtt!168
  - Removes pending message count (espressif/esp-mqtt@da6d38a)
  - Bugfix: Dispatch transport error on all write operations (espressif/esp-mqtt@5729048)
  - Merge enqueue functions (espressif/esp-mqtt@72833c7)
* Adds a configuration for outbox data destination
  - See merge request espressif/esp-mqtt!166
  - feat: Adds a configuration for outbox data destination (espressif/esp-mqtt@2c71f9e)
* CI: Add configuration for ttfw
  - See merge request espressif/esp-mqtt!167
  - See commit espressif/esp-mqtt@d71dcf3
* mqtt5: Fix flow control will increase count when send fragmented packet
  - See merge request espressif/esp-mqtt!164
  - Closes espressif/esp-mqtt#255
  - See commit espressif/esp-mqtt@5cce2c4
* Add license information to queue
  - See merge request espressif/esp-mqtt!163
  - See commit espressif/esp-mqtt@36f0faa
* MQTTv5: Fixes and additions from GitHub PRs
  - See merge request espressif/esp-mqtt!162
  - Merges espressif/esp-mqtt#250
  - fix: Allow MQTT v5 zero length payload (espressif/esp-mqtt@65a4fda)
  - feature: Include subscribe_id in esp_mqtt5_event_property_t (espressif/esp-mqtt@1011e63)
* Feature:  Enable SUBSCRIBE to multiple topics
  - See merge request espressif/esp-mqtt!156
  - See commit espressif/esp-mqtt@3210255
* Adds Kconfig option to configure poll read timeout
  - See merge request espressif/esp-mqtt!159
  - Closes: espressif/esp-mqtt#245
  - See commit espressif/esp-mqtt@2fa945d
* Fix formatting when using printf nano
  - See merge request espressif/esp-mqtt!160
  - See commit espressif/esp-mqtt@e9b865e
* mqtt5: Fix flow control will regard the DUP packet and not consider PUBCOMP packet
  - See merge request espressif/esp-mqtt!158
  - Closes espressif/esp-mqtt#243
  - See commit espressif/esp-mqtt@ed76036
* Remove possible null pointer dereferences
  - See merge request espressif/esp-mqtt!157
  - Bugfix: Remove Remove possible null pointer dereferences (espressif/esp-mqtt@f80772b)
CommanderRedYT pushed a commit to CommanderRedYT/esp-idf that referenced this issue Sep 24, 2023
git log --oneline dffabb067fb3c39f486033d2e47eb4b1416f0c82..301bd9e0282eb993523b84d6c56d2559346aa071

Detailed description of the changes:
* fix: Added missing update to message data
  - See merge request espressif/esp-mqtt!189
  - See commit espressif/esp-mqtt@cc41d1b
* PR: fixing typos in `mqtt5_error_reason_code`
  - See merge request espressif/esp-mqtt!188
  - feat: Add enum definition with typo to keep backwards compatibility (espressif/esp-mqtt@90b4a45)
  - fixing typos in `mqtt5_error_reason_code` (espressif/esp-mqtt@dc775bb)
* docs: Clarify keepalive timeout
  - See merge request espressif/esp-mqtt!186
  - See commit espressif/esp-mqtt@cb1e6cf
* fix: LOG format strings
  - See merge request espressif/esp-mqtt!187
  - See commit espressif/esp-mqtt@a3b04f2
* Fix: Stop client only if it's running.
  - See merge request espressif/esp-mqtt!183
  - See commit espressif/esp-mqtt@36eec6f
* fix: Error on publish message creation was ignored.
  - See merge request espressif/esp-mqtt!185
  - See commit espressif/esp-mqtt@585e3ba
* Fix: Allocation for connection buffer was incorrectly done.
  - See merge request espressif/esp-mqtt!182
  - See commit espressif/esp-mqtt@6c849c6
* Adds mqtt host tests to Ci
  - See merge request espressif/esp-mqtt!181
  - See commit espressif/esp-mqtt@4050df4
* Fix: Outbox was leaked in case of initialization failure
  - See merge request espressif/esp-mqtt!180
  - See commit espressif/esp-mqtt@5d491a4
* feat: Add option to bind interface of use
  - See merge request espressif/esp-mqtt!179
  - Closes espressif/esp-mqtt#253
  - See commit espressif/esp-mqtt@363fbf7
* Add outbox size control feature
  - See merge request espressif/esp-mqtt!141
  - feat: Introduces outbox limit (espressif/esp-mqtt@372ab7b)
  - Removes unused outbox functions. (espressif/esp-mqtt@21a5491)
  - refactor: Group access to output buffer in mqtt_connection_t (espressif/esp-mqtt@122875b)
* Add custom transport configuration
  - See merge request espressif/esp-mqtt!169
  - feat: Add custom transport configuration (espressif/esp-mqtt@a5c1b44)
* Removes leftover calls to event_handler
  - See merge request espressif/esp-mqtt!178
  - See commit espressif/esp-mqtt@a492935
* PR: Added support to set server common name.
  - See merge request espressif/esp-mqtt!173
  - Added support to set server common name. (espressif/esp-mqtt@6195762)
* Merge branch 'bugfix/return_on_qos0_disconnected' into 'master'
  - See merge request espressif/esp-mqtt!175
  - See commit espressif/esp-mqtt@5bd9724
* Merge branch 'bugfix/cpp_compilation' into 'master'
  - See merge request espressif/esp-mqtt!174
  - See commit espressif/esp-mqtt@70cbaca
* bug: Incorrect return on disconnect qos0 publish
  - See merge request espressif/esp-mqtt!172
* Fix: Compilation in C++ with multiple subscribe
  - See merge request espressif/esp-mqtt!171
* ci: Fix qemu build against 5.1
  - See merge request espressif/esp-mqtt!170
  - See commit espressif/esp-mqtt@94defb8
* Minor cleanups on mqtt client
  - See merge request espressif/esp-mqtt!168
  - Removes pending message count (espressif/esp-mqtt@da6d38a)
  - Bugfix: Dispatch transport error on all write operations (espressif/esp-mqtt@5729048)
  - Merge enqueue functions (espressif/esp-mqtt@72833c7)
* Adds a configuration for outbox data destination
  - See merge request espressif/esp-mqtt!166
  - feat: Adds a configuration for outbox data destination (espressif/esp-mqtt@2c71f9e)
* CI: Add configuration for ttfw
  - See merge request espressif/esp-mqtt!167
  - See commit espressif/esp-mqtt@d71dcf3
* mqtt5: Fix flow control will increase count when send fragmented packet
  - See merge request espressif/esp-mqtt!164
  - Closes espressif/esp-mqtt#255
  - See commit espressif/esp-mqtt@5cce2c4
* Add license information to queue
  - See merge request espressif/esp-mqtt!163
  - See commit espressif/esp-mqtt@36f0faa
* MQTTv5: Fixes and additions from GitHub PRs
  - See merge request espressif/esp-mqtt!162
  - Merges espressif/esp-mqtt#250
  - fix: Allow MQTT v5 zero length payload (espressif/esp-mqtt@65a4fda)
  - feature: Include subscribe_id in esp_mqtt5_event_property_t (espressif/esp-mqtt@1011e63)
* Feature:  Enable SUBSCRIBE to multiple topics
  - See merge request espressif/esp-mqtt!156
  - See commit espressif/esp-mqtt@3210255
* Adds Kconfig option to configure poll read timeout
  - See merge request espressif/esp-mqtt!159
  - Closes: espressif/esp-mqtt#245
  - See commit espressif/esp-mqtt@2fa945d
* Fix formatting when using printf nano
  - See merge request espressif/esp-mqtt!160
  - See commit espressif/esp-mqtt@e9b865e
* mqtt5: Fix flow control will regard the DUP packet and not consider PUBCOMP packet
  - See merge request espressif/esp-mqtt!158
  - Closes espressif/esp-mqtt#243
  - See commit espressif/esp-mqtt@ed76036
* Remove possible null pointer dereferences
  - See merge request espressif/esp-mqtt!157
  - Bugfix: Remove Remove possible null pointer dereferences (espressif/esp-mqtt@f80772b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants