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

Adds missing configuration fields #557

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

euripedesrocha
Copy link
Collaborator

Some aspects of the underlying mqtt client were not set.

Some aspects of the underlying mqtt client were not set.

Fix espressif#554
Copy link
Contributor

@gabsuren gabsuren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few explanatory lines will be good to have for the functions

@euripedesrocha
Copy link
Collaborator Author

euripedesrocha commented Apr 24, 2024

LGTM, just a few explanatory lines will be good to have for the functions

@gabsuren For which functions?

Comment on lines +136 to +151
mqtt_client_cfg.session.keepalive = config.session.keepalive;
mqtt_client_cfg.session.last_will.msg = config.session.last_will.lwt_msg;
mqtt_client_cfg.session.last_will.topic = config.session.last_will.lwt_topic;
mqtt_client_cfg.session.last_will.msg_len = config.session.last_will.lwt_msg_len;
mqtt_client_cfg.session.last_will.qos = config.session.last_will.lwt_qos;
mqtt_client_cfg.session.last_will.retain = config.session.last_will.lwt_retain;
mqtt_client_cfg.session.protocol_ver = config.session.protocol_ver;
mqtt_client_cfg.session.disable_keepalive = config.session.disable_keepalive;
mqtt_client_cfg.network.reconnect_timeout_ms = config.connection.reconnect_timeout_ms;
mqtt_client_cfg.network.timeout_ms = config.connection.network_timeout_ms;
mqtt_client_cfg.network.disable_auto_reconnect = config.connection.disable_auto_reconnect;
mqtt_client_cfg.network.refresh_connection_after_ms = config.connection.refresh_connection_after_ms;
mqtt_client_cfg.task.priority = config.task.task_prio;
mqtt_client_cfg.task.stack_size = config.task.task_stack;
mqtt_client_cfg.buffer.size = config.buffer_size;
mqtt_client_cfg.buffer.out_size = config.out_buffer_size;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use the visitor paradigm as for the broker options?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like: config_session(mqtt_client_cfg, config.session); ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly because here we have used the struct fields as is in the old mqtt client config struct. I'll do an update of the wrapper as a follow-up, but want to provide the fix first.

Copy link
Collaborator

@david-cermak david-cermak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a question about different config sections...

@euripedesrocha euripedesrocha merged commit 06d013b into espressif:master Apr 26, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants