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

IDF v5.0 regression: mqtt not providing user_context field (IDFGH-9261) #10644

Closed
3 tasks done
jmattsson opened this issue Jan 30, 2023 · 3 comments
Closed
3 tasks done
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally

Comments

@jmattsson
Copy link

Answers checklist.

  • I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
  • I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
  • I have searched the issue tracker for a similar issue and not found a similar issue.

General issue report

Hi,

I'm working on porting the NodeMCU mqtt module from IDFv4 to IDFv5, but I've run into what seems to be a big issue.

Previously*, it was possible to set a "user context" pointer when initialising the client, and that pointer would then be handed back in each event. This followed the typical pattern for event driven libraries in C, where the caller is free to associate whatever context with a session and have it accessible at the time of handling the event.

In IDFv5**, I cannot find any support for this. Is it really intentional that such a fundamental aspect of the library has been changed/removed? I'm looking at a significant rewrite of the module code here if that's the case. By the looks of it I would have to establish and maintain an external esp_mqtt_client_handle_t to user context lookup structure in order to achieve what the old user_context field did. Obviously, I'd prefer to not have to do that.

Have I overlooked something here? I can't find any mention of this change in the API documentation, release notes or migration notes.

I originally posted this on the forum but got no response there. This seems like quite a regression to me.

Thanks in advance.

*) See https://docs.espressif.com/projects/esp ... parameters
**) See https://docs.espressif.com/projects/esp ... figuration

@espressif-bot espressif-bot added the Status: Opened Issue is new label Jan 30, 2023
@github-actions github-actions bot changed the title IDF v5.0 regression: mqtt not providing user_context field IDF v5.0 regression: mqtt not providing user_context field (IDFGH-9261) Jan 30, 2023
@david-cermak
Copy link
Collaborator

Hi @jmattsson

Yes, the field user_context was removed intentionally, since mqtt client uses IDF standard esp-event, which allows adding its own user context. It's briefly described in the examples:

/* The last argument may be used to pass data to the event handler, in this example mqtt_event_handler */
esp_mqtt_client_register_event(client, ESP_EVENT_ANY_ID, mqtt_event_handler, NULL);

(note that user_context was used before migration to esp-event when notifications were posted as callbacks, and was kept for compatibility up to v5.x)

@jmattsson
Copy link
Author

Oh! Okay, that explains a lot. Thanks @david-cermak !
I hadn't seen any deprecation messages about the callbacks, so I was oblivious to the fact it'd been changed. I went back and checked all the release notes, and there is no mention of that anywhere that I can find. May I suggest adding a note to at least the 4->5 migration doc about this change before someone else gets similarly confused? :)
On my end I only saw errors about missing user_context and the changed esp_mqtt_client_config_t structure, which made me stare at the wrong things and miss the bigger picture.

Thanks again for the quick response. Feel free to close the issue (unless you want it to hang around for doc update reasons).

@AxelLin
Copy link
Contributor

AxelLin commented Jan 31, 2023

migration-guides should mention this change.

@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new Resolution: NA Issue resolution is unavailable labels Feb 2, 2023
espressif-bot pushed a commit that referenced this issue Mar 17, 2023
update CN for migration-guides/release-5.x/5.0/protocols.rst
Co-Authored-By: Wang Zi Yan <[email protected]>

Closes #10644
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests

4 participants