-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
samples: nrf9160: serial LTE modem sample #1099
Conversation
Found the following issues, please fix and resubmit: Gitlint issuesCommit 10ca9eeb07: |
CONFIG_SNTP=y | ||
|
||
# MQTT lib | ||
CONFIG_MQTT_SOCKET_LIB=y |
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.
Some early feedback - the upstream MQTT library (CONFIG_MQTT_LIB
) is slightly updated copy of the downstream library (CONFIG_MQTT_SOCKET_LIB
), and we plan to remove the downstream copy. Could you use the upstream version instead? You can check #391 to see what steps were needed to transition nRF Cloud library to use the upstream MQTT implementation.
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 project used to use upstream MQTT and later adapted to downstream MQTT_SOCKET. No problem I'll change back to previous one, which need to update code accordingly, too.
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 and sorry for the confusion, I'm struggling with downstream MQTT copy removal for a way too long...
Thank you! For your convenience you can check out the |
Beat the robot after 4 commits :) Will prepare a clean commit after review. |
@lemrey Thank you! It's a good practice to correct the code style following the guideline |
|
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 didn't manage to go through all files yet, will finish on Monday. Looks pretty good so far!
@@ -0,0 +1,125 @@ | |||
.. _serial_lte_modem : |
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.
@b-gent @carlescufi I'm not sure about the serial-lte-modem spec.docx
file provided. It contains valuable information about the sample but is quite large, so I'm not sure if it could be easily converted to .rst
format easily. Do we allow such supplement documentation in the repository?
Also, the |
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.
Still a few more files left, will be back to the review soon.
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.
Finally finished the review :)
It looks pretty good, left a few more comments to consider.
I have a question though. I understand that this is a fully functional application for nRF91, but to get the full image of the proposed solution, one would also need some kind of host application (possibly for nRF5) that would communicate over serial with the LTE modem sample. Do you have any plans to submit such a complementary sample as well (even a simple one)?
|
||
/* Test MQTT connection */ | ||
#ifdef CONFIG_THIN_MQTT_SERVICE | ||
/* th_mqtt_control(CMD_TYPE_MQTT_CONNECT, MQTT_CLIENT, */ |
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.
Remove dead code, please.
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.
This is left as reference code as we cannot test MQTT and LwM2M simultaneously.
I'll add comments for this.
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.
In that case how about guarding it with
#if defined(CONFIG_THIN_MQTT_SERVICE) && !defined(CONFIG_THIN_LWM2M_SERVICE)
?
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 advice, applied.
#include <net/socket.h> | ||
|
||
/* Partly copy/paste from zephyr/subsys/net/ip/utils.c | ||
* Cannot be used when BSD library selects NET_RAW_MODE |
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's no longer the case, you should be able to use Zephyr implementation now (inet_pton
).
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.
Tested inet_pton(), not working
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 need to investigate this, we use inet_ntop
in several samples, both should work.
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.
Ok, so I've tested it a bit and it seems that inet_pton
converts the address string correctly. The only difference is that the inet_pton
returns 1 on success (don't ask me why, IMO it's very confusing, but it's standardized here).
So the return value would need some extra attention here (return 0 if inet_pton
returns 1, and for instance -EINVAL
otherwise, IMO there's no need for more sophisticated error handling).
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.
Actually I got 1 yesterday and thought it fails :)
Now I changed it as you suggested and tested OK.
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.
@carlescufi @b-gent Still need to settle about the user guide committed in |
@carlescufi Shall we link the repo with sample clients in the documentation (see #1099 (review)). The repo contains only Keil projects. |
@junqingzou Thank you for updating the PR. There are still a few details to determine, I'd also ask you to squash your commits before we can merge. |
Please wait with merging until I can check the docs (tomorrow) |
@b-gent Sure thing, take your time :) |
No, I do not think so. This is a question for @ru-fu and @barsok |
Yes, definitely, we should. Even though they are not NCS clients at least they can serve as a starting point for users. |
I do not think so either, we cannot require our customers to have MS Word available to open a .docx file. |
I think we should ask the author, @junqingzou to just convert this to |
@carlescufi The spec doc is too much to be converted to .rst. Another option could be to put it with nRF52 sample clients, maintained here. @rlubos Please DO NOT merge this for now. PMT is asking for raw AT-based serial modem, for which I have another project. This protocol-based project is mostly for exposing the app protocols. There are still things left to be clarified by PMT before decision of what to be in NCS. PS I'll rebase this project to latest master with a clean submit later. |
The NCS documentation is in RST, so all documentation related to the samples must be converted to RST. Btw, it sounds like this sample covers a lot of features, so should it be an application instead of a sample? (Samples should only show one single feature, while applications show a full use case.) |
* nRF9160 (UART2) | ||
|
||
* UART configuration | ||
* Hardware flow control: enabled |
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.
Tabulation slipped in (before enabled
)
#define INVALID_DESCRIPTOR -1 | ||
|
||
#define AT_BUF_SIZE CONFIG_INTER_CONNECT_UART_BUF_SIZE | ||
#define THREAD_STACK_SIZE 1048 |
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 suppose 1024 was meant here? Or is this finetuned to 1048?
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.
You could use KB(x)
from zephyr/include/sys/util.h
here, if 1048
is a mistake.
Otherwise a short comment could clarify that 1048
is correct.
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.
It's a typo and updated to KB(1), Thanks.
} | ||
|
||
/* Read AT socket in non-blocking mode. */ | ||
r_bytes = recv(at_socket_fd, at_buf, AT_BUF_SIZE - 4, |
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.
Why AT_BUF_SIZE - 4
and not simply AT_BUF_SIZE
?
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.
4 is the reserved size for protocol overhead (STX, TYPE, LENGTH, BCC). Actually it should be 5, plus the Notification type as the buffer is reserved for receive.
* @param param Parameter data for command. | ||
* @param length Length of parameter data. | ||
*/ | ||
void th_tcpip_control(u8_t cmd, const u8_t *param, uint8_t length); |
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.
uint8_t length
-> u8_t length
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.
Updated
#include "th_connect.h" | ||
#include "th_util.h" | ||
|
||
#define TCPIP_BUFFER_SIZE (CONFIG_INTER_CONNECT_UART_BUF_SIZE - 4) |
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.
Why - 4
?
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.
4 is the reserved size for protocol overhead (STX, TYPE, LENGTH, BCC). Actually it should be 5, plus the Notification type as the buffer is reserved for receive.
ret = recv(client.sock, rx_buffer, | ||
TCPIP_BUFFER_SIZE, | ||
0); | ||
if (ret < 0) { |
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.
We should also handle a situation when peer disconnects (ret == 0
). In that case we should no longer use the socket - it should be closed and client.connected
set to false.
I'd actually handle receive errors (ret < 0
) in the same way, as they typically render the socket unusable.
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.
Updated. The close of socket would be in DISCONNECT
int ret; | ||
|
||
memcpy(&port, param, 2); | ||
address_use_ip = *(param+2) & ADDRESS_TYPE_MASK; |
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.
Spaces around +
here and below.
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.
Updated
memcpy(&port, param, 2); | ||
address_use_ip = *(param+2) & ADDRESS_TYPE_MASK; | ||
address_length = *(param+2) & ADDRESS_LENGTH_MASK; | ||
memcpy(address, param + 3, address_length); |
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.
Why's the address copied in UDP case, and not in TCP? I'd unify the behavior unless there's a reason to do it differently.
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.
TCP has address handled in CONNECT. UDP has no CONNECT so has to handle address for every sendto and recvfrom.
sizeof(struct sockaddr)); | ||
if (ret < 0) { | ||
LOG_ERR("sendto() failed: %d", -errno); | ||
return -errno; |
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.
We should close the socket here, otherwise, we leak a socket. Or break
instead of return
.
|
||
default: | ||
err = -EINVAL; | ||
LOG_ERR("unknown %d", cmd); |
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.
"unknown command %d"? Lone "unknown" might be confusing when seen in logs.
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.
Updated
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.
It looks good! I left many comments because it's large and there's many files involved but there's nothing critical except the handling of out of memory errors, which needs to be fixed imo.
|
||
#include "th_connect.h" | ||
|
||
#define CONFIG_UART_0_NAME "UART_0" |
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.
Don't define macros prefixed with CONFIG_
, they might be mistaken for Kconfig-generated macros.
LOG_ERR("Missing STX"); | ||
return; | ||
} | ||
#if CONFIG_INTER_CONNECT_BCC |
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.
Prefer IS_ENABLED
when possible, to make sure this code will keep being compile-able; it will be optimized out in the end.
ARG_UNUSED(work); | ||
|
||
if (data_handler_cb == NULL) { | ||
LOG_ERR("Not itialized"); |
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.
Typo: initialized
uart_dev = device_get_binding(uart_dev_name); | ||
if (uart_dev == NULL) { | ||
LOG_ERR("Cannot bind %s", uart_dev_name); | ||
return -EINVAL; |
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.
Perhaps -EIO
is a better choice here.
int err; | ||
enum select_uart uart_id = CONFIG_INTER_CONNECT_UART; | ||
|
||
if (data_handler == NULL) { |
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.
Consider using __ASSERT
instead, if this function is not called directly by the user / part of the interface.
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.
Here and other places where you check for initialization, see whether that can happen if the user misuses the interface or not. If it can't then it should be an __ASSERT
.
|
||
#define STACKSIZE 1024 | ||
#define MQTT_BUF_SIZE (CONFIG_INTER_CONNECT_UART_BUF_SIZE - 4) | ||
#define PRIORITY K_PRIO_PREEMPT(CONFIG_THIN_MQTT_THREAD_PRIO) |
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.
Why is this thread preemptive instead of cooperative? I ask because most (all?) threads in Zephyr are cooperative so I think we should have a good reason to let them be otherwise.
if (c->transport.type == MQTT_TRANSPORT_NON_SECURE) { | ||
fds.fd = c->transport.tcp.sock; | ||
} else { | ||
#if defined(CONFIG_MQTT_LIB_TLS) |
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.
Is it possible to use IS_ENABLED
here?
#ifdef CONFIG_THIN_MQTT_BROKER_USER_NAME | ||
if (strlen(CONFIG_THIN_MQTT_BROKER_USER_NAME) > 0) { | ||
client.user_name = k_malloc(sizeof(struct mqtt_utf8)); | ||
__ASSERT_NO_MSG(client.user_name != 0); |
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.
We shouldn't handle out of memory errors with __ASSERT
, if it is a fatal error to be out of memory, we should allocate that memory statically. Otherwise, we should handle out of memory errors without crashing the application.
Please make sure to also fix this elsewhere.
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 a side note, when comparing against pointers use NULL
.
if (strlen(CONFIG_THIN_MQTT_BROKER_USER_NAME) > 0) { | ||
client.user_name = k_malloc(sizeof(struct mqtt_utf8)); | ||
__ASSERT_NO_MSG(client.user_name != 0); | ||
client.user_name->utf8 = k_malloc(64); |
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.
Why 64?
|
||
/**@brief Function to unsubscribe to the configured topic | ||
*/ | ||
int do_mqtt_unsubscribe(const u8_t *topic, u8_t length) |
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.
Why do you declare *topic
as const
and then down cast it on line 465?
If it needs indeed to be constant, down casting is dangerous 💣
If it doesn't need to be constant, you can remove the const
and the cast on line 465.
All checks are passing now. checkpatch (informational only, not a failure)
Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
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, that was a lot of new code. I've left some feedback in the comments.
|
||
config SLM_AT_MODE | ||
bool "Serial LTE Modem by raw AT mode" | ||
default 'y' |
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.
Alignment
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.
Updated
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.
This one is still unaligned.
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.
Sorry, Editor miss. Updated
# | ||
config SLM_TCPIP_AT_MODE | ||
bool "Support TCP/IP by raw AT mode" | ||
default 'y' |
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.
Alignment
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.
Updated
# | ||
config SLM_GPS_AT_MODE | ||
bool "Support GPS by raw AT mode" | ||
default 'y' |
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.
Alignment
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.
Updated
int slm_at_tcpip_parse(const u8_t *at_cmd, u8_t length); | ||
|
||
/** | ||
* @brief Initialize GPS AT command parser. |
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.
GPS -> TCP/IP
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.
Updated
|
||
/**@brief Socket operations. */ | ||
enum slm_socket_operation { | ||
AT_SOCKET_CLOSE, |
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.
Indent with tabulation, not spaces - here and in slm_tcpip_at_cmd_type
.
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.
Updated
|
||
/**@brief List of supported AT commands. */ | ||
enum slm_gps_mode { | ||
GPS_MODE_STANDALONE, |
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.
Tabs for indentation, here and below.
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.
Updated
* @retval 0 If the operation was successful. | ||
* Otherwise, a (negative) error code is returned. | ||
*/ | ||
int at_host_init(void); |
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.
We already have this API in NCS. If the sample needs to reimplement this functionality, please use different name (or just prefix with slm_
as other APIs here.
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.
Updated to slm_at_host_init
nrf_gpio_cfg_sense_set(CONFIG_SLM_MODEM_WAKEUP_PIN, | ||
NRF_GPIO_PIN_SENSE_LOW); | ||
|
||
/* The LTE modem also needs to be stopped by issuing a command*/ |
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 think it would look nicer to have a single, multiline comment here:
/*
* ...
*/
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.
Updated
@@ -0,0 +1,316 @@ | |||
/** |
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 don't think we should introduce non-NCS samples into NCS codebase. @carlescufi Can you comment on that, was that agreed before?
@junqingzou I see that the sample was completely rewritten, and now supports AT-like commands for custom modules (gps/tcpip etc). We already have an at_client
sample in NCS, are you familiar with it? Perhaps it would be possible to reuse it's codebase and prepare a NCS version of the client application?
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.
Sorry for the misleading name "at_client", which is actually nRF52 project to work with the Serial LTE Modem. I'll remove it from this PR. Serial LTE Modem could work with either PC terminal or MCU code. "at_client" is a sample to run it in MCU code.
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.
@rlubos Serial LTE Modem is copied from nrf\lib\at_host then add what PMT asked for. There might be more local AT commands to add based on customer requests.
#endif | ||
} | ||
|
||
#ifdef CONFIG_SLM_GPIO_WAKEUP |
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.
@carlescufi Could anyone more familiar with low-power modes in nRF have a look at this part?
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.
@pizi-nordic and @anangl could you please take a look at this particular part?
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, I've noticed a few buffer overflows in this round that would need to be fixed.
Also, in order to finalize this PR, all of the checkpatch errors need to be fixed. While not critical, fixing warnings is also highly appreciated.
client.ip_proto = IPPROTO_IP; | ||
ret = -errno; | ||
} else { | ||
sprintf(buf, "#XSOCKET: %d, %d\r\n", client.sock, client.ip_proto); |
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.
This string is at minimum 16 bytes + null terminator, this will exceed the allocated buf
size.
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.
Like the gps code, use a global 64-byte buffer now.
offset += ret; | ||
} | ||
|
||
sprintf(buf, "#XUDPSENDTO: %d\r\n", offset); |
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, this string will be longer than 16 bytes.
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.
Like the gps part, use a global 64-byte buffer now.
} else if (*(at_param) == '?') { | ||
char buf[16]; | ||
if (client.sock != INVALID_SOCKET) { | ||
sprintf(buf, "#XSOCKET: %d, %d\r\n", client.sock, |
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.
Also here, more than 16 bytes.
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.
Like the gps part, use a global 64-byte buffer now.
|
||
config SLM_AT_MODE | ||
bool "Serial LTE Modem by raw AT mode" | ||
default 'y' |
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.
This one is still unaligned.
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, looks good from my side.
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.
Left a few comments.
static char buf[64]; | ||
|
||
#define THREAD_STACK_SIZE KB(1) | ||
#define THREAD_PRIORITY K_PRIO_PREEMPT(7) |
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.
Why specifically 7, why preemptive?
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.
No special consideration here. 7 means the same as main thread. What's the recommended priority for application threads? Also did not consider in depth on preemptive. Thought to just pick one between 0...NUM_PREEMPT_PRIORITIES(15)
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 main thread default priority is not 7.
config MAIN_THREAD_PRIORITY
int "Priority of initialization/main thread"
default -2 if !PREEMPT_ENABLED
default 0
help
Priority at which the initialization thread runs, including the start
of the main() function. main() can then change its priority if desired.
I suggest using a low priority: K_LOWEST_APPLICATION_THREAD_PRIO
.
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 project has "CONFIG_MAIN_THREAD_PRIORITY=7" in the prj.conf, which was copied from the MQTT sample long time ago. Anyway, I tried K_LOWEST_APPLICATION_THREAD_PRIO and confirmed it's good so I'll change to use it. Thanks a lot!
LOG_ERR("Failed to stop GPS (err: %d)", -errno); | ||
ret = -errno; | ||
} else { | ||
k_thread_abort(gps_thread_id); |
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.
side note: this will prevent the thread from running again until you reset..
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.
Changed to use k_thread_suspend()/k_thread_resume().
Smoke test shows it works. Please review again.
****************** | ||
|
||
* AT#XSOCKET=<op>,<type> | ||
* AT%XSOCKET? |
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.
Small typo: "AT%XSOCKET?" --> "AT#XSOCKET?"
Eagerly looking forward to this branch being finished and merged into master :-)
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.
Thank you. Also missing "AT#XBIND" in README.rst. Will be added.
@lemrey Your comments seem to be addressed, could you revisit? |
#. nRF91 is woken up and sends a ``Ready\r\n`` message to the nRF52. | ||
#. On receiving the message, nRF52 can proceed to issue AT commands. | ||
|
||
You can find a sample nRF52 client project in the :file:`client` folder, which can be put under "SDK_15.3.0/examples/peripheral" folder. |
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.
Please help me understand why we are suddenly talking about nRF5 SDK here?
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.
Sorry this is a left-over of old code, where a nRF52 project is kept inside this nRF91 project. It's removed based on review comment. I changed the line like this "A sample nRF52 client project is to be shared by other means."
I added a doc review commit, please check the diff and let me know if my changes are correct. If yes, then squash the doc commit. |
} | ||
|
||
/**@brief Irrecoverable BSD library error. */ | ||
void bsd_irrecoverable_error_handler(uint32_t err) |
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.
We have now removed the bsd_irrecoverable_error_handler()
, from bsdlib.
This should be removed.
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.
Rebased to BSDlibv0.5.0 and have this function removed from main.c.
Thanks!
Please take a look at the CI failures. |
I fixed (hopefully) the ones coming from this PR. Two of them are unrelated and come from a user guide. |
I added another commit that fixes some CI issues and adds a Testing section. Please have a look and squash. My last comment is about the statement in line 140: It sounds super cryptic and I doubt it adds any value. Unless you have a good reason to keep it there, I'd suggest to remove. Generally, this looks good to me as it is now. |
There seems to be a wrong merge in links.txt. |
That is right, check the file links.txt on master, around line 100: The entry is removed on this branch and causes doc CI to fail. |
Hi @junqingzou, |
Lifting the review till we sort out the command parsing
@rlubos yes I confirmed it's related to the change in this commit) |
@junqingzou, I've proposed a fix for the AT parser in #1359. With this change, the This + rlubos@7989d75 fixes the command parsing in the sample. Please let me know if you're fine with the changes, and feel free to squash it into your commit. |
I added a commit that should fix the doc CI , let's see if it works now. |
The Serial LTE Modem (SLM) project is an enhancement to at_client sample - support TCP/IP proprietary AT commands - support GPS proprietary AT commands - support communication to external MCU over UART Like original at_client, all modem AT commands are still supported. More proprietary AT commands could be added in the future. Signed-off-by: Jun Qing Zou <jun.qing.zou@nordicsemi.no>
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.
LGTM
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.
LGTM, tested the sample and it works fine 👍
Hello there. I still can't understand why with this example with frequent AT command sending nrf9160 bricks. AT client example works normally, but serial lte modem sample bricks very easily, just try to send few AT commands with less than 100 ms delay... |
Hi - questions like this will get lost here. Please ask at https://devzone.nordicsemi.com/ and our support team can help you. |
The Serial LTE Modem (SLM) project is an enhancement to at_client sample
- support TCP/IP proprietary AT commands
- support GPS proprietary AT commands
- support communication to external MCU over UART