-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Library: MQTT-SN Client library Gateway Search Support #80021
base: main
Are you sure you want to change the base?
Library: MQTT-SN Client library Gateway Search Support #80021
Conversation
Hello @SpontaneousDuck, and thank you very much for your first pull request to the Zephyr project! |
146ac49
to
e2ca93a
Compare
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.
Thanks for the contribution, some initial feedback from my side. Looks pretty neat overall.
subsys/net/lib/mqtt_sn/mqtt_sn.c
Outdated
char addr[CONFIG_MQTT_SN_LIB_MAX_ADDR_SIZE]; | ||
size_t addr_len; |
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.
What's the expected address format? What byte order?
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 was expecting the addresses to be agnostic to the library and it would only store the address and pass pointers back to the transport code the user creates. Does the byte order matter then?
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.
Sure, but the address format for a specific transport should still be known for the application, I'm not sure if it's documented anywhere right now.
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 could add some documentation to that that the user will need to define this based on their implemented transport?
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.
That would be nice.
zassert_equal(err, 0, "unexpected error %d"); | ||
zassert_equal(client->state, 1, "Wrong state"); | ||
zassert_equal(evt_cb_data.called, 1, "NO event"); | ||
zassert_equal(evt_cb_data.last_evt.type, MQTT_SN_EVT_CONNECTED, "Wrong event"); | ||
k_sleep(K_MSEC(10)); | ||
} | ||
|
||
static ZTEST(mqtt_sn_client, test_mqtt_sn_connect_no_will) | ||
static ZTEST(mqtt_sn_client, test_mqtt_sn_handle_advertise) |
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.
New test cases could be in a separate commit.
@@ -0,0 +1,6 @@ | |||
This folder contains an example MQTT-SN Gateway made for use with a native_sim board application utilizing MQTT-SN. |
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.
Same here, convenience configs could be introduced in a separate commit.
@@ -43,25 +44,65 @@ static int tp_udp_init(struct mqtt_sn_transport *transport) | |||
{ | |||
struct mqtt_sn_transport_udp *udp = UDP_TRANSPORT(transport); | |||
int err; | |||
struct sockaddr_in addrm; |
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.
So no IPv6 support?
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.
Not currently in the provided implementation but that matches the current implementation that I am extending. This does not preclude the user from implementing their own transport that uses IPv6. Should I add support as part of this PR or leave the current implementation's limitation?
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.
As the other parts of the Zephyr network stack typically support IPv6 and IPv4 (dual stack), it would be preferable if all new code also support dual stack. So yes, I would prefer IPv6 support also in this PR.
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.
Added IPv6 support I believe with a switch at the two places it matters.
What would be the recommended way to add changes based on code comments? It appears like the desired commits to merge will be all the code changes in one commit with test cases and convenience commits in different commits (3 total). Is this incorrect and I can add the requested changes in another commit? Or should I |
Typically you just squash the review fixes into original commits (you can use As for splitting the changes into separate commits, I guess the easiest would be to And yes, you'd need to force-push as the history is rewritten, but that's a common thing to do in Zephyr PRs. Don't worry about review comments, GH will handle this. |
e2ca93a
to
50e046c
Compare
Fixes: zephyrproject-rtos#78010 This commit implements the "Gateway Advertisement and Discovery" process defined in section 6.1 of the MQTT-SN specification. This includes breaking changes to the transport interface and the default included UDP interface implementation as support for UDP multicast messages is added as implemented by the Paho MQTT-SN Gateway. Signed-off-by: Kenneth Witham <[email protected]>
Fixes: zephyrproject-rtos#78010 This commit adds tests for the "Gateway Advertisement and Discovery" process. This includes tests for handling SEARCHGW, GWINFO, and ADVERTISE as well as manually adding a Gateway for use. Gateway pruning when missing ADVERTISE messages is also tested. Signed-off-by: Kenneth Witham <[email protected]>
Fixes: zephyrproject-rtos#78010 This commit adds the "Gateway Advertisement and Discovery" process to the MQTT-SN Publisher Sample application. Signed-off-by: Kenneth Witham <[email protected]>
50e046c
to
b92b926
Compare
Ok, I think I made it through all the comments and pushed all the changes up in 3 new commits. One for main code and doc changes, one with unit tests, and one with the sample code updates. |
This pull requests extends the MQTT-SN Client library to support the "Gateway Advertisement and Discovery" process defined in section 6.1 of the MQTT-SN specification. This includes breaking changes to the transport interface and the default included UDP interface implementation as support for UDP multicast messages is added as implemented by the Paho MQTT-SN Gateway (link).
This PR closes issue #78010.
Changes List: