ESP Modem #469
ESP Modem #469
Conversation
DaneSlattery
force-pushed
the
dms_modem
branch
from
0a37e21 to
7801c39
Compare
|
Right now I'm seeing some events on the This is what my console shows so far, I'm not sure yet if the modem is fully able to connect to the LTE network as I haven't tried to do some network request. |
There was a problem hiding this comment.
Most important in my feedback is that we need to think what is EspModem's purpose in life, so to say?
I.e. shall we hard-wire it to Espressif MCUs and to UART (as it is right now), or can we do something (a) more flexible, that supports more protocols than just UART (b) make it useful in contexts beyond ESP IDF (i.e. embassy-net and Linux) (c) Make it pure-Rust and ideally having life on its own, outside of esp-idf-svc?
This way, a future sim-modem crate or something might become the way on embedded, to do the "pre-PPP" AT-commands-based negotiation with various supported modems and can be used in many more contexts than just what you are currently aiming at - esp32 with the modem connected over UART.
src/modem.rs
Outdated
| @@ -0,0 +1,335 @@ | |||
| use at_commands::{builder::CommandBuilder, parser::CommandParser}; | |||
There was a problem hiding this comment.
I'm not sure we need EspModem necessarily inside esp-idf-svc. There might be a brighter future for it, where it can be made to work with multiple platforms, including ESP-IDF, embassy-net-ppp and with Linux PPP (the last one for easy testing).
There was a problem hiding this comment.
This is how I understand the SimModem will work. I want to be able to keep track of which mode the device is in using command mode and data mode.
I also want to provide the user code access to the Read and Write interface so that they can hook up the ppp rx and tx through SimModem
I think the SimModem will also be generic over type T, where T: DCE. DCE will be a new introduced trait, with a single impl for SimComA7600, which takes care of getting the device into data mode. I also think we should allow for send_cmd to be public so that you can still do things like:
get the iccid,
get the imei
get the signal strength
There was a problem hiding this comment.
Not really sure what is "DTE" on that picture? For your modem, the "terminal" is just the pair of embedded_io::Read + embedded_io::Write. Why do you need anything else here?
I also want to provide the user code access to the
ReadandWriteinterface so that they can hook up the ppprxandtxthroughSimModem
I do not understand this. It is the user code that will give your sim modem the Read + Write traits, so the user code apriori has access to those. You only need to do something special if/when you plan to support MUX mode, where somehow on top of this user-provided Read+Write you'll implement two "virtual" tunnels:
- One, for commands, but you keep this inside the modem
- Another, for PPP (data) -> this you need to expose to the users
But maybe we need to think about this once you have support for MUX? Because at the moment, in the context of "either-commands-or-data" you don't need to provide to the user anything...
I think the
SimModemwill also be generic over typeT, whereT: DCE. DCE will be a new introduced trait, with a single impl forSimComA7600, which takes care of getting the device into data mode.
Isn't this the same as my PppRunner thing?
By the way, I see you are copying the terminology from the C ESP modem. No offense, but this terminology is highly confusing... DTE, DCE... yeah.
I also think we should allow for
send_cmdto be public so that you can still do things like: get the iccid, get the imei get the signal strength
Sure no prob.
There was a problem hiding this comment.
Isn't this the same as my PppRunner thing?
By the way, I see you are copying the terminology from the C ESP modem. No offense, but this terminology is highly confusing... DTE, DCE... yeah.
No not quite. PPPRunner describes how to feed the tx and rx. ModemDevice(the C api calls this DCE) describes how to get a specific device into data mode. For this case SIM7600 sends those specific commands, but not every modem will have the same formatting, delimiters, responses or newlines etc.
DTE is any "terminal", but essentially a device that implements embedded_io::Read+embedded_io::Write
There was a problem hiding this comment.
Ah, ok. Instead of having multiple SimModems, like struct Sim7600Modem, struct Br96Modem, struct Sim800Modem where these all implement a common trait SimModem trait, you want to model it slightly differently.
OK.
There was a problem hiding this comment.
I opted to keep it simpler. SimModem is a trait with
negotiate() and get_mode(). Can probably add a few more general things such as signal strength, ICCID, IMEI.
EspModem stays as a kind of glue between EspNetifDriver and UartDriver
src/modem.rs
Outdated
|
|
||
| pub struct EspModem<'d, T> | ||
| where | ||
| T: BorrowMut<UartDriver<'d>>, |
There was a problem hiding this comment.
If EspModem (under a new name - say - LteModem or SimModem) is to become platform-neutral, rather than taking an ESP-IDF specific UartDriver, it should take a pair of embedded_io::Read + embedded_io::Write trait impls (and in future, also a variant where it takes their async variants).
Like this:
pub struct SimModem<R, W> {
read: R,
write: W,
}There is a better reason to consider generifying EspModem/SimModem though: is UART the only possible connectivity scenario with the modem?
SIM7600 supports USB as a connectivity, so with the esp32s2 / esp32s3 connecting the modem over USB might be a much better option, as these MCUs claim to support USB 2.0 full-speed, which is ~ 480MBIT 12MBIT. With UART, and its theoretical max baud rate of 5MBIT on the Esps, you might be unable to take advantage of the max modem speed anyway? What is it for the SIM7600? Maybe a few tens of MBITs, like 30-50MBIT or even 150MBIT? (Update not that with USB you'll be able to fully take advantage of it, but it would still be faster than 5MBIT maybe?)
Update 2: Also, other modems might support I2C or SPI. Not sure myself how a communication over I2C/SPI can be layered / represented as Read/Write traits - we need to check that and if possible in the context of sim modems, Read/Write might be the correct abstraction still.
There was a problem hiding this comment.
Update 3: esp32p4 seems to even support USB 2.0 high-speed, i.e. 480MBIT
| // start ppp | ||
| self.set_data_mode()?; | ||
|
|
||
| // now in ppp mode. |
There was a problem hiding this comment.
This subscription to raw ESP IDF events is hopefully necessary only for testing and will be removed in the end.
There was a problem hiding this comment.
Yes this is valid. I do think the subscription is currently duplicated in netif.rs, but I think we should keep it there and remove it here.
src/modem.rs
Outdated
| cmd.parse(Ok(&buff[..len])) | ||
| } | ||
|
|
||
| pub fn setup_data_mode(&mut self, sysloop: EspSystemEventLoop) -> Result<(), EspError> { |
There was a problem hiding this comment.
Given that this function does more than just setting up data mode - i.e. it "runs" the PPP stack RX/TX loop, I suggest to rename it to simply "run". It should also run "forever" or until the PPP TX/RX loop exits because it had lost connection to the modem, or because the loop receives a PPP frame that designates the modem had exited.
There was a problem hiding this comment.
... or - if we decide that EspModem/SimModem should only be platform-neutral, "pure Rust" and only dealing with the "pre-PPP" phase, we can leave it named setup_data_mode OR maybe negotiate.
There was a problem hiding this comment.
I have created a run loop, but the only issue is that it mutably borrows. I need to work around that so that one can call "is_connected" etc
I've also moved the SimModem into a module of it's own with the intention to separate into a crate once I can get the embedded_io::Read to read without blocking forever.
There was a problem hiding this comment.
@ivmarkov any ideas for the mutable borrow here. I want to run the run loop in a separate thread (spawned in userland), but I also want to do things like EspModem.wait_netif_up() after the run loop, but this requires another borrow. You can see my failed attempts in the examples/lte_modem.rs. Is it time to pull out the Arc<Mutex>?
There was a problem hiding this comment.
Using an Arc<Mutex<...>> externally to EspModem::run will be impossible, because the mutex will lock the modem for as long as the run method "runs" so to say, which is not what we want.
So if we have any interior mutability (i.e. Mutex or similar) - which I don't think is avoidable at all - it has to be internal to the modem struct itself.
I think we have two options:
- Option A: (I have a slight preference for this) Turn
run(&mut self)intorun(&self)and then internally useMutexes as needed (i.e. interior mutability) - Option B: Introduce
EspModem::split(&mut self) -> (EspModemRunner<'_>, EspModemStatus<'_>), whereEspModemRunnerjust has aEspModemRunner::run(&mut self)andEspModemStatushas whatever you need in terms of status (is_connectedetc.). Here, you would needArconly ifEspModemRunnerandEspModemStatusare not lifetimed by the mutable borrow ofsplit(&mut self)
Given that both options will likely require some form of interior mutability after all (i.e. a Mutex or suchlike) I would say perhaps Option A?
There was a problem hiding this comment.
I like option A, although it seems like we would need to internally Arc+mutex the netif and serial members
src/modem.rs
Outdated
| reset(comm)?; | ||
| fn negotiate( | ||
| &mut self, | ||
| comm: &mut UartDriver, |
There was a problem hiding this comment.
You are returning the UartDriver where you already had a nice abstraction just because you need to do a read-with-a-timeout.
This is unnecessary. Reading with a timeout is as simple as combining Read with - say - embedded_hal_async::Delay like so:
async fn read_with_timeout<R: embedded_io_async::Read, D: embedded_hal_async::DelayNs>(read: R, delay: D, buf: &mut [u8], timeout_ms: u16) -> Result<usize, R::Error> {
let read = read.read(buf);
let timeout = delay.delay_ms(timeout_ms as _);
match embassy_futures::select::select(read, timeout).await {
Either::First(result) => result,
Either::Second(_) => Ok(0), // 0 bytes read indicates a timeout
}
}There was a problem hiding this comment.
@DaneSlattery Ah sorry!
I did not realize your code still lives in "blocking"-land and not in async-land yet. :D Perhaps we can delay the re-introduction of embedded_io(_async)::Read + Write until after we start to switch this code from blocking to async! For now, maybe just make it work with UartDriver (where the you-assume-that-your-read-reply-will-contain-the-whole-response issue is the more important issue), and once we have something mostly working reliably, we can switch from blocking to async. And then still layer blocking on top of async (Or use AsyncUartDriver maybe).
There was a problem hiding this comment.
(
The issue at hand is that stuff like read_with_timeout is much, much more difficult to implement in blocking-land (would require scoped threads) than it is in async-land. This is one place where async is strictly more powerful than blocking - the "intra-task concurrency" from the blog of Boats, if you recall that.
)
There was a problem hiding this comment.
I saw your comments on the PR, and I was only using the timeout to force UartDriver:::read to return the bytes. I was confident that after 1s the modem will have replied, but that is completely arbritrary. I like the solution to feed parts into a Digester that is a state machine searching for some terminator (like '\r\nOK\r\n'.as_bytes(). This can happen after a few read cycles. After the terminator is found, then we can try parse it using the at_commands parser. If the terminator is not found, the digester will keep waiting. It would actually be really useful if at_commands::parser could provide expects_terminator(...) and throw a different error if the terminator is not found.
src/modem.rs
Outdated
| .named("+CSQ") | ||
| .finish()?; | ||
|
|
||
| comm.write(cmd).map_err(|_| ModemError::IO)?; | ||
|
|
||
| let len = comm.read(&mut buff).map_err(|_| ModemError::IO)?; | ||
| let len = comm |
There was a problem hiding this comment.
I don't think this type of reading (and all others in all get_* methods below and above) is reliable.
You are not taking into account, that you might not get the complete response as a single shot to read. Here for example, the fact that you get \r\n+CSQ: 19,99\r\n\r\nOK\r\n out of a single read is a pure coincidence, and might break any time. Just think what would've happened, if I've kept the other code in UartDriver::read, which did return - on an empty ringbuffer - a single byte after waiting for it. Havoc.
I think you need a utility method that probably takes a callback closure of sorts, where the callback closure is containing the parsing code of CommandParse::parse and then - in the absence of a reliable end-of-command-response termination character (like a CR or LF) that you might wait for - is trying to apply parsing on the buffer repeatedly, and if the data in the buffer is incomplete - tries to read a bit more at the end of the buffer.
For that purpose, you might want to take a look at embedded_io::BufRead as its API is handy for such cases (and implementing BufRead over Read is trivial, if not done already in the lib itself).
Alternative: you might want to layer a async fn which reads line-by-line from a BufRead and refactor your parsing to line-by-line (i.e. CommandParser would expect lines somehow).
Reading line-by-line simply means you call BufRead::read repeatedly until you notice a sequence of \r\n in the buffer, then consume everything until (and trimming) \r\n and pass this as the "line" to your stateful parser of sorts.
There was a problem hiding this comment.
A third alternative (but I find this really ugly) and it will make the negotiation slower, is to read in your buffer - in a loop - until a given timeout. Where the expectation is that until this timeout had expired, the modem would've replied - in your buffer - with the complete response to your command.
But of course this introduces an arbitrary variable in all of this (the "read till you get the whole response" timeout, which would be - what? - a trial and error exercise).
There was a problem hiding this comment.
Looking at the atat crate, here is a similar "loop" to what I mean. Basically, the loop reads at the end of the buffer and then hands the whole buffer to a "digester". The digester would return DigestResult::None if the data in the buffer is "incomplete" (i.e. we have to read more to get the whole response).
There was a problem hiding this comment.
I think you need a utility method that probably takes a callback closure of sorts, where the callback closure is containing the parsing code of CommandParse::parse and then - in the absence of a reliable end-of-command-response termination character (like a CR or LF) that you might wait for - is trying to apply parsing on the buffer repeatedly, and if the data in the buffer is incomplete - tries to read a bit more at the end of the buffer.
I do think at least that \r\nOK\r\n is a reliable end of command termination.
There was a problem hiding this comment.
... and probably \r\nError\r\n or suchlike in case of errors? (Not sure such a thing exists but I could imagine if it can answer with "ok" it can probably answer with something else too.)
There was a problem hiding this comment.
Not sure what is the difficulty with BufRead but here's something which returns stuff line by line (with a simpler terminator indeed, but you can extend to a more complex one):
use core::fmt;
use std::io::{self, BufRead};
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub enum Line<'a> {
End(&'a [u8]),
Continue(&'a [u8]),
}
impl<'a> Line<'a> {
pub fn len(&self) -> usize {
self.slice().len()
}
pub fn slice(&self) -> &[u8] {
match self {
Self::End(slice) => slice,
Self::Continue(slice) => slice,
}
}
pub fn str(&self) -> &str {
let s = match self {
Self::End(slice) => &slice[..slice.len() - 1], // Remove the trailing`\n`
Self::Continue(slice) => slice,
};
core::str::from_utf8(s).unwrap() // Should return an IO error instead
}
}
impl<'a> fmt::Display for Line<'a> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
Self::End(_) => write!(f, "End({})", self.str()),
Self::Continue(_) => write!(f, "Continue({})", self.str()),
}
}
}
pub fn read_line<'a, R: BufRead>(read: &'a mut R) -> io::Result<Option<Line<'a>>> {
let slice = read.fill_buf()?;
if slice.is_empty() {
return Ok(None);
}
let line = slice
.iter()
.enumerate()
.find_map(|(index, byte)| (*byte == b'\n').then_some(index + 1))
.map_or_else(|| Line::Continue(slice), |len| Line::End(&slice[..len]));
Ok(Some(line))
}
pub fn main() -> io::Result<()> {
let lines: &[u8] = b"foo\nbar\na really long line\nand then another one";
let mut read = io::BufReader::new(lines);
while let Some(line) = read_line(&mut read)? {
println!("Line `{line}`");
let line_len = line.len();
read.consume(line_len);
}
Ok(())
}There was a problem hiding this comment.
(Update: removed the comment above Line::Continue as you actually can't get rid of it as the BufRead::fetch_buf does not guarantee you to fetch extra, if there is already content in the buffer. The only way to recover whole lines from the above example code is by appending continue+continue+...+end in a large enough buffer. Or by switching your parsing logic to an iterator-like approach where you process byte-by-byte (= char-by-char for ascii).
There was a problem hiding this comment.
Be mindful of the last response to the command with which you switch to PPP. There you might have to read byte-by-byte, or else you might read a bit into the first PPP packet right after the end of the last AT response. Speculating of course, not sure if the modem will start sending PPP packets immediately, but you might need to check that.
With this requirement, I think we will need a byte-by-byte approach anyway, at least for the last packet.
The above is wrong.
consume needs to physically remove amt bytes from the beginning of the buffer (i.e. the bytes of &buf[..amt]), by doing an overlapping-copy of &buf[amt..pos] (src) into &buf[..pos - amt] (dest).
And then you need to do self.pos -= amt;.
I think I fundamentally didn't understand BufRead, but I think I get it now.
Why? UartDriver already does implement Read, so BufReader<N, UartDriver<'d>> would just work.
Yes you are right. I have implemented a BufReader targeting embedded-io. rust-embedded/embedded-hal#627, that should work with UartDriver.
As for the approach that uses a raw Read (read_until_term)... I'm not sure it is correct either. The problem is, the call to T::read might read data beyond your line terminator (i.e. data from the next line). You need to preserve this data somewhere. And then re-use it in the next call to read_until_term.
In this case, i'm not sure that is so much of an issue because the AT commands are request ->response, although I see some mention of the idea of "URC" or Unsolicited Result Code, but I haven't encountered them yet, at least for SIM7600.
Not sure what is the difficulty with BufRead but here's something which returns stuff line by line (with a simpler terminator indeed, but you can extend to a more complex one):
BufRead seemed to be returning the entire buffer[self.pos..], including the un-initialized bits. I think I still prefer an approach that reads the entire buffer until \r\nOK\r\n is found, but I am still working on it.
There was a problem hiding this comment.
BufRead seemed to be returning the entire
buffer[self.pos..], including the un-initialized bits. I think I still prefer an approach that reads the entire buffer until\r\nOK\r\nis found, but I am still working on it.
No, BufRead::fill_buf should - by definition - only return a slice from its internal buffer, which contains valid, initialized data (and all of it; that is - all of it that had not yet been consumed).
With that said, I DO agree that at least because of the last request-response which happens right before the channel starts talking PPP, you might have to switch to a byte-by-byte consumption of the input data, because otherwise you risk "eating up" the beginning of the first incoming PPP packet into the internal buffer of BufRead (though I would check if PPP really operates in a way where it might start sending you data immediately after the response to your last AT command).
OR - and maybe that's better? - you can just continue utilizing the same BufRead object that was initially used for AT commands also for the PPP stream? That's completely possible, and BTW layering Read over BufRead should be trivial I think (though maybe bearing an unnecessary overhead as with PPP you don't need the "naive" overlapping copy, that the hidden BufRead would inevitably do).
But... your call. :)
There was a problem hiding this comment.
I think we will have to create a new BufReader in negotiate() that is passed into each of the read/write parsing functions.
|
@DaneSlattery because this is a large effort with many moving parts, I hope you don't mind if I merge a portion of the changes already now? What I think is ready for merge is your "nonstatic" enhancements to To have you listed as a contributor to this code in GIT history and yet - to save you some time - I'll fork your PR branch, revert the other changes which are still not ready ( Once the changes land in But please tell me if you prefer with me to wait until we have everything in-place instead. I'm still not sure how I feel specifically about the |
I am happy to get this in so long, go ahead, I'll rebase once the dust is settled. I have started porting stuff to a I saw your comments on the PR, and I was only using the timeout to force
Go ahead. I do think |
DaneSlattery
force-pushed
the
dms_modem
branch
2 times, most recently
from
fc6d0f6 to
8303a7d
Compare
DaneSlattery
force-pushed
the
dms_modem
branch
from
6c55b0b to
0121093
Compare
Attempt at #468