-
Notifications
You must be signed in to change notification settings - Fork 135
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
feat(esp_modem): Add mqtt example in AT-only mode #156
feat(esp_modem): Add mqtt example in AT-only mode #156
Conversation
Hi @david-cermak, very interesting PR. |
@jonathandreyer Thanks for the interest. Yes, I also like this idea of the localhost listener, since we're able to use standard IDF libraries (the same way as we do for WiFi/Ethernet) and even using the IDF TLS stack (with mbedTLS)! The disadvantage is that these socket commands are (very) different for different devices. My plan is to look at other devices and try to add some abstraction layer. It would be helpful if you could test it then, once we add support for Another point we'd like to improve is the composition of commands for specific devices, so we could add custom commands as well as include certain type of commands (like addons), e.g. for socket commands, GNSS commands or as you mentioned the MQTT commands. It's party addressed in this PR, but I wanted to make the modem factory more general, so we're able to build/create a DCE including commands from these groups (general V.25TER, specific control commands, network commands,...). Once this is supported, it would be very helpful if contributors implement these command groups for their devices and their use-cases. |
@david-cermak Thanks for your reply. I fully agree about the abstraction and the customization of commands. |
Hi, agree that URC is important. |
This PR actually addresses these asynchronous messages as the notifications about the received messages come as URC. At this point only the Rx messages, but need to watch for socket close and others... esp-protocols/components/esp_modem/examples/modem_tcp_client/main/sock_dce.cpp Lines 276 to 285 in a299192
|
i would suggest to have a more general URC Handling, independend of this PR. i will look into it. |
Hello @david-cermak, I have tried to use this WIP PR, but I wasn't able to build sources. Into the build log, it seems that a file is missing ("socket_commands.inc"). Is this possible or is it a problem on my side? |
Oh, sorry, forgot to add this file into my initial commit. Have just added. |
Hello @david-cermak, thanks for the reply and the fix. I have created a rebase branch (jonathandreyer/esp-protocols:feature/modem_example_at_mqtt) with few fix to be able to build it with CI and also be able to run it with v4.4. |
Hello @david-cermak, I have updated the branch with an initial for the BG96 modem. Don't hesitate if you have any comment? In this initial commit, I didn't modify functions tcp_send() and tcp_recv(), because in the current version it is not used. |
Hello @jonathandreyer Thank you for your help with promoting this PR from just an idea to the real implementation, and supporting BG96 modem! I think I'd clean this up and make it ready for review (here I would borrow your ci-fix commit). Then it should be easier for you to raise a new PR with the BG96 support. |
74d9b95
to
4d2bd34
Compare
Note the last commit that support fragmented received data. This allows receiving/sending bigger chunks which are typically send during TLS handshake. (MQTT over TLS didn't work before this fix). As for supporting other devices, it's quite simple to build an abstraction on these commands: esp-protocols/components/esp_modem/examples/modem_tcp_client/main/sock_commands.cpp Lines 36 to 40 in 588a73c
But, not so easy for the Tx/Rx state machine, such as: esp-protocols/components/esp_modem/examples/modem_tcp_client/main/sock_dce.cpp Lines 113 to 132 in 588a73c
Will think about it more, but if you have an idea or suggestion, please let me know |
components/esp_modem/include/cxx_include/esp_modem_command_library.hpp
Outdated
Show resolved
Hide resolved
explicit Creator(std::shared_ptr<DTE> dte): dte(std::move(dte)), device(nullptr), netif(nullptr) | ||
{ | ||
} | ||
|
||
~Creator() |
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.
Unrelated: By declaring this destructor move operations are not declared by the compiler. Could we achieve this error LOG by other means?
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.
The meaning of this error message is that some part, added to the factory was left behind, unused for constructing. There's no easy fix I know about. The only thing is to make the factory
state-less, but that would need complete refactor.
char *recv_data = (char *)data; | ||
if (data_to_recv == 0) { | ||
const std::string_view head = "+CIPRXGET: 2,0,"; | ||
auto head_pos = (char *)std::search(data, data + len, head.begin(), head.end()); |
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.
Have you considered making a string_view
from data as an alternative to the usage of memchr
and pointer manipulation?
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 have, but since the replay contains not only string delimiters, but also and mainly data (and that could be very long data), I'd prefer using memchr
and treating this as raw binary, rather than string_view
.
As you can see, some portions of those data are parsed using string_view
, but that doesn't universally work for the entire data we receive.
|
||
using namespace esp_modem; | ||
|
||
command_result net_open(CommandableIf *t) |
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.
Could t
have a better name?
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.
Have renamed to term
(this could be any class, that implements commands and as such, we call it "commandable", but there are in most cases just more or less abstracted terminals
components/esp_modem/examples/modem_tcp_client/main/sock_commands.cpp
Outdated
Show resolved
Hide resolved
components/esp_modem/examples/modem_console/main/my_module_dce.cpp
Outdated
Show resolved
Hide resolved
@@ -22,6 +22,14 @@ namespace dce_commands { | |||
* @{ |
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.
Unrelated SPDX-FileCopyrightText: 2021-2022
-> SPDX-FileCopyrightText: 2021-2023
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 sure why the pre-commit hook for copyrights didn't trigger. Let me check.
components/esp_modem/examples/modem_tcp_client/main/modem_client.cpp
Outdated
Show resolved
Hide resolved
components/esp_modem/examples/modem_tcp_client/main/modem_client.cpp
Outdated
Show resolved
Hide resolved
components/esp_modem/examples/modem_console/main/modem_console_main.cpp
Outdated
Show resolved
Hide resolved
9e3770b
to
68d6bf1
Compare
68d6bf1
to
64f178c
Compare
Thanks for the review @euripedesrocha @gabsuren @espressif-abhikroy |
MAJOR CHANGE: Enable DTE to redefine on_read() and write(cmd) directly
Similar to pppos-client, but without PPP mode. This uses standard mqtt client and a loopback service that forwards the TCP traffic from localhost to the modem and vice-versa. (next step is to create a dedicated tcp-transport layer for modem) Console example uses DCE commandable to demontrate how to handle URC
Plus rework a little to support implementation of multiple devices
64f178c
to
31d4323
Compare
Similar to the
pppos-client
, but without PPP mode.This uses standard mqtt client and a loopback service that forwards the TCP traffic from localhost to the modem and vice-versa. (next step is to create a dedicated tcp-transport layer for modem)