-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[embassy-net-nrf91] patch greater rx_data_len size #4098
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
Conversation
The modem firmware tells us the list length for rx. It would be better to just use it, instead of validating it equals 16 or 32. This is better because
It'll need a small refactor to not use a fixed-length array for the rx list. Could you do that? |
@Dirbaio thanks for your fast reply. I too first thought that in the code we could just remove the assert_eq but it comes with faults: a change or defect in the modem would go unnoticed; e.g. a faulty value, wrong read address or error in the unsafe access could cause greater errors later on. As the LIST_LEN is used at other parts of the code too, it could cause problems if a length is read that is not divide-able by 8/16/32. With the feature the check is still in place. If you still suggest to just use the the given list length from the modem, add this comment a 🚀 and i refactor it to not use a fixed-length array. |
Yes, we really should change it. There's two lists:
Nordic's lib uses the len provided by the modem. When I initially implemented embassy-net-nrf91, I saw the RX list length seemed to be always 16 and decided to just hardcode it because it was easier. With modem firmware 2.0.0 (available for nrf9151, not for nrf9160) they changed the list length to 32. |
New version is looking good! :) Seems something went wrong while merging/rebasing, there's now 300 commits in the PR. Could you fix that? I'd also suggest squashing it down to one commit so the previous approach doesn't show up in the git history. |
b372056
to
fb986a2
Compare
cleaned and squashed the PR with those changes i can access and communicate with both the nrf9160 and nrf9151 modem (verified trough AT commands). Sadly i only get the nrf9160 to establish a connection but not the nrf9151. the nrf9151 loops endless at currently i can not establish a LTE/NB-IoT connection with the nrf9151 and opened a question regarding this in the Nordic DevZone: https://devzone.nordicsemi.com/f/nordic-q-a/122021/nrf9151-replaces-nrf9160-lte-not-working-any-more |
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.
General feedback and comments explaining the changes
@@ -42,6 +42,7 @@ docserver-builder -i ./embassy-usb-logger -o webroot/crates/embassy-usb-logger/g | |||
docserver-builder -i ./embassy-usb-synopsys-otg -o webroot/crates/embassy-usb-synopsys-otg/git.zup | |||
|
|||
docserver-builder -i ./embassy-net -o webroot/crates/embassy-net/git.zup | |||
docserver-builder -i ./embassy-net-nrf91 -o webroot/crates/embassy-net-nrf91/git.zup |
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.
embassy net nrf91 is missing in the embassy docs. this should add it so we have a nice and up to date documentation available online
embassy-net-nrf91/src/context.rs
Outdated
@@ -48,6 +48,8 @@ pub enum Error { | |||
AtParseError, | |||
/// Error parsing IP addresses. | |||
AddrParseError, | |||
/// Timed out waiting for network attach. | |||
AttachTimeout, |
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.
additionally add a optional Timeout, that if the modem connection timer (usually 60 seconds) clocks out, the modem connection process is handled gracefully and returns a readable error
embassy-net-nrf91/src/context.rs
Outdated
@@ -66,7 +68,7 @@ pub struct Status { | |||
/// Gateway if assigned. | |||
pub gateway: Option<IpAddr>, | |||
/// DNS servers if assigned. | |||
pub dns: Vec<IpAddr, 2>, | |||
pub dns: Vec<IpAddr, 3>, |
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.
according to docs and comments, three DNS addresses were supposed
embassy-net-nrf91/src/context.rs
Outdated
@@ -143,13 +145,17 @@ impl<'a> Control<'a> { | |||
.map_err(|_| Error::BufferTooSmall)?; | |||
let _ = self.control.at_command(op, &mut buf).await; | |||
// Ignore ERROR which means no pin required | |||
trace!("SIM PIN configured"); | |||
} else { | |||
trace!("No SIM PIN to send"); |
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.
more versatile log output if trace is active
embassy-net-nrf91/src/context.rs
Outdated
const MAX_TIMEOUT: Duration = Duration::from_secs(60); | ||
|
||
let mut elapsed = Duration::from_secs(0); | ||
loop { |
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 modem attach timeout logic
assert!(shmem_len != 0); | ||
trace!(" shmem_ptr = {}, shmem_len = {}", shmem_ptr, shmem_len); | ||
|
||
assert!(shmem_len != 0, "shmem length must not be zero"); |
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.
return users readable error feedback
@@ -202,6 +217,8 @@ async fn new_internal<'a>( | |||
|
|||
rx_control_list: ptr::null_mut(), | |||
rx_data_list: ptr::null_mut(), | |||
rx_control_len: 0, | |||
rx_data_len: 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.
added new variable RX lengths
trace!("modem control list length: {}", rx_control_len); | ||
trace!("modem data list length: {}", rx_data_len); | ||
self.rx_control_len = rx_control_len; | ||
self.rx_data_len = rx_data_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.
get and set the RX Lengths we got from the modem
} else { | ||
self.rx_data_len | ||
}; | ||
for i in 0..max { |
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.
either who is in control gets to tell the iteration length
embassy-net-nrf91/src/lib.rs
Outdated
// AT response | ||
3 => self.handle_resp(msg), | ||
// AT notification | ||
4 => false, | ||
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.
get some more useful tracing logs about AT notifications
Hi @diondokter you were recommended to me and i saw you wrote the nrf-modem crate and did some embassy work here too. could you please take a look into this? |
Hi, I've never looked at the internals of libmodem properly, so I don't have a lot of info on that front. Not sure what I can help with... |
thanks for your lightning fast reply. could you verify the changes of this PR work on a NRF9151? i got a hard time testing and figuring out the internals of the modem and NRF and only got a custom PCB instead of the DevBoard therefore i want to make sure this works and does not throw up elsewhere. especially connecting to LTE/NB-IoT Networks |
I only have an nRF9160 :( |
fb986a2
to
83c9ea0
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.
The changes look good, but I'd need to get a nrf9151 to verify it.
fyi, I got hold of an nrf9151, so I will give it a spin in the coming days. |
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 finally got around to test it, and it works nicely on my nrf9151-dk. Nice work! I also verified it still works on my nrf9160 DK.
I left one comment regarding the timeout that I think needs to be configurable, as I had to adjust that for the attach to succeed.
embassy-net-nrf91/src/context.rs
Outdated
trace!("waiting for attached"); | ||
// Poll every second, up to 60 seconds, then error out | ||
const POLL_INTERVAL: Duration = Duration::from_secs(1); | ||
const MAX_TIMEOUT: Duration = Duration::from_secs(60); |
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 should be configurable, maybe via some kind of config type passed when creating the context. Because in my case it can take longer than 60 secs, and I've seen some cases where it could take up to 20 minutes to attach in testing (probably some kind of rate limiting if you attach too frequently).
00c144c
to
a8e905f
Compare
Thanks for the PR! I've rebased it and removed unrelated things to the main point of the PR (nrf9151)
|
Switching from a NRF9160 MCU to the newer NRF9151 there were some changes regarding LTE connections. I used the same code on both boards.
0
and reboot. In the same step it's also recommended and verified to usehfclk_source = HfclkSource::Internal
in your init config, e.g.:Except of course if you got an explicit external clock in use.
rx_control_len
from uint16
to32
. Changing this leads to usage as usual.Tested on the NRF9160-DK and a NRF9151 testboard.
Done:
TODO: examples to test and add to examples folder (possibly other PR in future)