Revisit on mutable self references #39
Comments
|
I think my example stands. If you can show me a functioning example which has at least two users of embedded-nal (e.g. an HTTP client and something else - CoAP, SMTP, DNS, MQTT, or whatever), and that the burden of juggling the mut-ref between two entirely independent protocol implementations isn't onerous, I'm happy to be persuaded. I come at this as a protocol and wireless systems engineer and so separation of concerns is important to me. Plus I prefer to place complexity in the nal impl rather than on the nal user, so it only has to be done once. I understand though, that others may have different priorities and if your model is, say, the Arduino HTTP library, I can see why embedded-nal might grate a little. I also think my example of the BSD socket library is valid, as it's what almost all stacks are based on (certainly Linux, Windows and macOS are). There, the mutable state is entirely an issue for your OS, and not your application or library - you do not have to lock anything, you just call 'open' or 'write', etc. This is what I have tried to follow, but using an immutable reference to alleviate the need for a global static state variable, which makes unit testing harder. |
|
In my opinion, the comparison with the BSD Socket API or POSIX (connect) API is not fair or completely correct. There is no equivalent argument for
In my understanding, if I need an instance to be mutable from two or more sources, I would then wrap it in a I see your point that it might become awkward managing and passing along wrapped driver impls. But I still think, writing a little (user-)wrapper is more flexible than enforcing support for all scenarios on the driver level. pub trait UdpClientStack {
fn send(&mut self, socket: &mut Self::UdpSocket, buffer: &[u8]) -> nb::Result<(), Self::Error>;
}
pub struct MyDriver { ... };
impl UdpClientStack for MyDriver {
fn send(&mut self, socket: &mut Self::UdpSocket, buffer: &[u8]) -> nb::Result<(), Self::Error> {
// do the stuff without overhead
}
}
pub struct RefWrapper<'a>(&'a RefCell<MyDriver>);
// pub struct MutexWrapper<'a>(&'a Mutex<MyDriver>); // as alternative in threaded environments
// pub struct RcWrapper(Rc<RefCell<MyDriver>>); // for alloc environments
impl UdpClientStack for RefWrapper<'_> {
fn send(&mut self, socket: &mut Self::UdpSocket, buffer: &[u8]) -> nb::Result<(), Self::Error> {
self.0.borrow_mut().send(socket, buffer)
}
}
fn main() {
let mut driver = MyDriver::new();
send(&mut driver); // no overhead
let shared_driver = RefCell::new(driver);
let mut http_ref = RefWrapper(&shared_driver);
let mut smtp_ref = RefWrapper(&shared_driver);
let mut mqtt_ref = smtp_ref.clone(); // or if clone is derived
send(&mut http_ref); // opt-in overhead
let mut client = http_connect(http_ref, "https://www.google.de/search?q=what+is+my+ip");
let ip = client.read(..);
}
fn send(udp: &mut impl UdpClientStack) {
// ...
}
// for the sake of typing-simplicity of this example, assuming HTTP travels over UDP
fn http_connect<T: UdpClientStack>(my_owned_stack: T, url: &str) -> HttpClient { ... }If I am not mistaken, the lifetimes of |
embedded-nal exists because I was annoyed it was so difficult to write an HTTP library that worked on both a Quectel BC68 (TCP over AT over UART) and a Nordic nRF9160 (BSDish sockets). I then didn't have time to finish the idea, so it ended up here. Your argument that we should bring the choice of which mutex (if any) to use up to the application layer, as opposed to putting it at the TCP/UDP layer is persuasive. It does make assembling an application more complicated (outside of the naive case of using a single NAL protocol), but perhaps all the tedious |
|
I think we can have a best-of-both-worlds situation if we do require a Thus, it would be up to the high-level application author whether they want runtime efficiency (passing the single |
|
@kellerkindt I think only In general I would say that a simple, single threaded application, where you don't need mutexes is probably not the majority and should probably not be the focus of |
|
I find @kellerkindt's comment about zero-cost-abstractions to be compelling. Requiring all driver implementations to be internally thread-safe puts a wasted cost on applications that don't need it. I also think he's right that it's unfair to compare an embedded driver to the interface provided by an operating system. IMO the NAL should dictate only the most simple common abstraction possible. The whole point of an abstraction layer is that higher level features (like convenient interfaces) can be implemented once for all abstraction implementations. A feature like thread safety (however important to whatever potential majority) falls outside of that simple abstraction. My thoughts on this align with what @Sympatron suggested: providing a thread-safe wrapper. It would need the following attributes:
Applications that need to share the driver would simply use the type constraint If this is a direction that is acceptable to the maintainers, I'd be willing to start experimenting into how this could be done. I don't expect it would be particularly complicated, but I'm still a junior rustacean at this point (especially when it comes to thread safety); it's possible there are issues I haven't yet considered. |
|
I would have no issue going in that direction, and would definitely be very keen on seeing the result. Can any of you think of other abstration crates that does it in this way? or the other way for that sake? Thoughts @ryan-summers @eldruin ? |
|
I personally am more aligned with @chrysn's opinion. Specifically, I do see a lot of applications that will only use a single protocol on top of the embedded-nal, where the NAL is intended just to be an abstract API so that a protocol isn't tied to a particular stack. However, as noted, some users may need to use it with multiple protocols, and forcing them to handle the synchronization is quite an ask (it's a complex, hard topic). I figure it may be worth stating here, but in the In SummaryIf someone can persuade me that a generic implementer of |
|
Replicating something like |
|
I think the object which hands out mutable access to the stack is necessarily going to depend on the RTOS (or not) that you are running.
Tangentially, this doesn't solve the issue that if the reference isn't immutable, you can't store the stack reference in the socket object. This means you need to be very careful to pass the correct socket object to the correct stack object. let mut buffer = [0u8; 1500];
let mut stack_one = QuectelTcpStack(uart1);
let mut stack_two = QuectelTcpStack(uart2);
let mut stack_three = PosixTcpStack();
let mut socket_one = stack_one.open("google.com", 80);
let results = stack_two.read(&mut socket_one, &mut buffer); /* boom - and even type checking can't help here */
let results = stack_three.read(&mut socket_one, &mut buffer); /* also boom - but type checking could catch this */In an ideal world you could do: let results = socket_one.read(&mut buffer);But, this is incompatible with the decision around using |
|
@thejpster While the current embedded-nal traits require internal mutability, they don't enforce thread safety. Right now whether a driver is thread-safe is entirely up to the implementation. Since that's the status quo, I think we should solve and release a single-thread solution, then come up with a thread-safe solution after. What do you think? Here's what I have so far on a single-threaded sharable wrapper, similar to shared-bus. Edit: stop, don't real all this code. Read the later revision: #39 (comment) use core::cell::RefCell;
use embedded_nal::{SocketAddr, UdpClient};
pub struct SharedUdpClientManagerSimple<T: UdpClient> {
driver: RefCell<T>,
}
impl<T: UdpClient> SharedUdpClientManagerSimple<T> {
pub fn new(driver: T) -> Self {
SharedUdpClientManagerSimple {
driver: RefCell::new(driver),
}
}
pub fn acquire(&self) -> SharedUdpClientSimple<T> {
SharedUdpClientSimple {
driver: &self.driver,
}
}
}
pub struct SharedUdpClientSimple<'a, T: UdpClient> {
driver: &'a RefCell<T>,
}
impl<'a, T: UdpClient> UdpClient for SharedUdpClientSimple<'a, T> {
type Error = T::Error;
type UdpSocket = T::UdpSocket;
fn connect(&mut self, address: SocketAddr) -> Result<Self::UdpSocket, Self::Error> {
self.driver.borrow_mut().connect(address)
}
fn send(
&mut self,
socket: &mut Self::UdpSocket,
data: &[u8],
) -> Result<(), embedded_nal::nb::Error<<T as embedded_nal::UdpClient>::Error>> {
self.driver.borrow_mut().send(socket, data)
}
fn receive(
&mut self,
socket: &mut Self::UdpSocket,
data: &mut [u8],
) -> Result<(usize, SocketAddr), embedded_nal::nb::Error<<T as embedded_nal::UdpClient>::Error>>
{
self.driver.borrow_mut().receive(socket, data)
}
fn close(&mut self, socket: Self::UdpSocket) -> Result<(), Self::Error> {
self.driver.borrow_mut().close(socket)
}
}This compiles nicely. This depends on an embedded-nal patched to use mutable references. Obviously this isn't super useful yet, since you can only share a single trait from the NAL at a time. I need to think more about how to permit sharing as many stack traits as are available. |
|
Makes sense. Should it be in this crate though? Or should it be in embedded-nal-sharing (name TBD)? |
|
@thejpster Different crate IMO. I'm using that same name for my testing crate. Took me a little while of lying in bed to realize how to share all the different traits. This wraps and shares all three traits nicely. Usage: let mut manager = SharedNalManager::new(/* initialize driver*/);
let driver1 = manager.acquire();
let driver2 = manager.acquire();
let tcp_socket = driver1.open();
let udp_server_socket = driver2.bind(/* ... */);
// ...Code: Edit: warning again, don't read all this. There's a better one below: #39 (comment) use core::cell::RefCell;
use embedded_nal::{nb, SocketAddr, TcpStack, UdpClient, UdpServer};
pub struct SharedNalManager<T> {
driver: RefCell<T>,
}
impl<T> SharedNalManager<T> {
pub fn new(driver: T) -> Self {
SharedNalManager {
driver: RefCell::new(driver),
}
}
pub fn acquire(&self) -> SharedNal<T> {
SharedNal {
driver: &self.driver,
}
}
}
pub struct SharedNal<'a, T> {
driver: &'a RefCell<T>,
}
impl<'a, T: UdpClient> UdpClient for SharedNal<'a, T> {
type Error = T::Error;
type UdpSocket = T::UdpSocket;
fn connect(&mut self, address: SocketAddr) -> Result<Self::UdpSocket, Self::Error> {
self.driver.borrow_mut().connect(address)
}
fn send(
&mut self,
socket: &mut Self::UdpSocket,
data: &[u8],
) -> Result<(), nb::Error<<T as embedded_nal::UdpClient>::Error>> {
self.driver.borrow_mut().send(socket, data)
}
fn receive(
&mut self,
socket: &mut Self::UdpSocket,
data: &mut [u8],
) -> Result<(usize, SocketAddr), nb::Error<<T as embedded_nal::UdpClient>::Error>> {
self.driver.borrow_mut().receive(socket, data)
}
fn close(&mut self, socket: Self::UdpSocket) -> Result<(), Self::Error> {
self.driver.borrow_mut().close(socket)
}
}
impl<'a, T: UdpServer> UdpServer for SharedNal<'a, T> {
fn bind(&mut self, local_port: u16) -> Result<Self::UdpSocket, Self::Error> {
self.driver.borrow_mut().bind(local_port)
}
fn send_to(
&mut self,
socket: &mut Self::UdpSocket,
remote: SocketAddr,
buffer: &[u8],
) -> nb::Result<(), Self::Error> {
self.driver.borrow_mut().send_to(socket, remote, buffer)
}
}
impl<'a, T: TcpStack> TcpStack for SharedNal<'a, T> {
type TcpSocket = T::TcpSocket;
type Error = T::Error;
fn open(&mut self) -> Result<Self::TcpSocket, Self::Error> {
self.driver.borrow_mut().open()
}
fn connect(
&mut self,
socket: Self::TcpSocket,
remote: SocketAddr,
) -> Result<Self::TcpSocket, Self::Error> {
self.driver.borrow_mut().connect(socket, remote)
}
fn is_connected(&mut self, socket: &Self::TcpSocket) -> Result<bool, Self::Error> {
self.driver.borrow_mut().is_connected(socket)
}
fn write(
&mut self,
socket: &mut Self::TcpSocket,
buffer: &[u8],
) -> nb::Result<usize, Self::Error> {
self.driver.borrow_mut().write(socket, buffer)
}
fn read(
&mut self,
socket: &mut Self::TcpSocket,
buffer: &mut [u8],
) -> nb::Result<usize, Self::Error> {
self.driver.borrow_mut().read(socket, buffer)
}
fn close(&mut self, socket: Self::TcpSocket) -> Result<(), Self::Error> {
self.driver.borrow_mut().close(socket)
}
} |
|
@kellerkindt ping for your feedback on this |
|
Tried my hand at writing a macro for the first time, and wow, between the intuitive syntax and the clear error messages, that was really easy. Got rid of the infinite list of Edit: realized I hadn't pulled in a while, so this is now up-to-date with jonahd-g/embedded-nal#no-internal-mutability use core::cell::RefCell;
use embedded_nal::{nb, SocketAddr, TcpClientStack, TcpFullStack, UdpClientStack, UdpFullStack};
pub struct SharedNalManager<T> {
driver: RefCell<T>,
}
impl<T> SharedNalManager<T> {
pub fn new(driver: T) -> Self {
SharedNalManager {
driver: RefCell::new(driver),
}
}
pub fn acquire(&self) -> SharedNal<T> {
SharedNal {
driver: &self.driver,
}
}
}
pub struct SharedNal<'a, T> {
driver: &'a RefCell<T>,
}
macro_rules! forward {
($func:ident($($v:ident: $IT:ty),*) -> $T:ty) => {
fn $func(&mut self, $($v: $IT),*) -> $T {
self.driver.borrow_mut().$func($($v),*)
}
}
}
impl<'a, T: UdpClientStack> UdpClientStack for SharedNal<'a, T> {
type Error = T::Error;
type UdpSocket = T::UdpSocket;
forward! {socket() -> Result<Self::UdpSocket, Self::Error>}
forward! {connect(socket: &mut Self::UdpSocket, address: SocketAddr) -> Result<(), Self::Error>}
forward! {send(socket: &mut Self::UdpSocket, data: &[u8]) -> Result<(), nb::Error<<T as embedded_nal::UdpClientStack>::Error>>}
forward! {receive(socket: &mut Self::UdpSocket, data: &mut [u8]) -> Result<(usize, SocketAddr), nb::Error<<T as UdpClientStack>::Error>>}
forward! {close(socket: Self::UdpSocket) -> Result<(), Self::Error>}
}
impl<'a, T: UdpFullStack> UdpFullStack for SharedNal<'a, T> {
forward! {bind(socket: &mut Self::UdpSocket, local_port: u16) -> Result<(), Self::Error>}
forward! {send_to(socket: &mut Self::UdpSocket, remote: SocketAddr, buffer: &[u8]) -> Result<(), nb::Error<<T as UdpClientStack>::Error>>}
}
impl<'a, T: TcpClientStack> TcpClientStack for SharedNal<'a, T> {
type TcpSocket = T::TcpSocket;
type Error = T::Error;
forward! {socket() -> Result<Self::TcpSocket, Self::Error>}
forward! {connect(socket: &mut Self::TcpSocket, address: SocketAddr) -> Result<(), nb::Error<<T as TcpClientStack>::Error>>}
forward! {send(socket: &mut Self::TcpSocket, data: &[u8]) -> Result<usize, nb::Error<<T as embedded_nal::TcpClientStack>::Error>>}
forward! {receive(socket: &mut Self::TcpSocket, data: &mut [u8]) -> Result<usize, nb::Error<<T as TcpClientStack>::Error>>}
forward! {is_connected(socket: &Self::TcpSocket) -> Result<bool, Self::Error>}
forward! {close(socket: Self::TcpSocket) -> Result<(), Self::Error>}
}
impl<'a, T: TcpFullStack> TcpFullStack for SharedNal<'a, T> {
forward! {bind(socket: &mut Self::TcpSocket, port: u16) -> Result<(), <T as TcpClientStack>::Error>}
forward! {listen(socket: &mut Self::TcpSocket) -> Result<(), <T as TcpClientStack>::Error>}
forward! {accept(socket: &mut Self::TcpSocket) -> Result<(<T as TcpClientStack>::TcpSocket, SocketAddr), nb::Error<<T as TcpClientStack>::Error>>}
} |
|
I don't think a `RefCell` is `Sync`, so it cannot be shared in multiple
contexts. I've personally been using Unsafe cell instead. Will take a
closer look at the ideas here later as well
…On Sun, Jan 17, 2021, 21:02 Jonah Dahlquist ***@***.***> wrote:
Tried my hand at writing a macro for the first time, and wow, between the
intuitive syntax and the clear error messages, that was really easy. Got
rid of the infinite list of self.driver.borrow_mut()s.
use core::cell::RefCell;use embedded_nal::{nb, SocketAddr, TcpStack, UdpClient, UdpServer};
pub struct SharedNalManager<T> {
driver: RefCell<T>,
}
impl<T> SharedNalManager<T> {
pub fn new(driver: T) -> Self {
SharedNalManager {
driver: RefCell::new(driver),
}
}
pub fn acquire(&self) -> SharedNal<T> {
SharedNal {
driver: &self.driver,
}
}
}
pub struct SharedNal<'a, T> {
driver: &'a RefCell<T>,
}
macro_rules! forward {
($func:ident($($v:ident: $IT:ty),*) -> $T:ty) => {
fn $func(&mut self, $($v: $IT),*) -> $T {
self.driver.borrow_mut().$func($($v),*)
}
}
}
impl<'a, T: UdpClient> UdpClient for SharedNal<'a, T> {
type Error = T::Error;
type UdpSocket = T::UdpSocket;
forward! {connect(address: SocketAddr) -> Result<Self::UdpSocket, Self::Error>}
forward! {send(socket: &mut Self::UdpSocket, data: &[u8]) -> Result<(), nb::Error<<T as embedded_nal::UdpClient>::Error>>}
forward! {receive(socket: &mut Self::UdpSocket, data: &mut [u8]) -> Result<(usize, SocketAddr), nb::Error<<T as embedded_nal::UdpClient>::Error>>}
forward! {close(socket: Self::UdpSocket) -> Result<(), Self::Error>}
}
impl<'a, T: UdpServer> UdpServer for SharedNal<'a, T> {
forward! {bind(local_port: u16) -> Result<Self::UdpSocket, Self::Error>}
forward! {send_to(socket: &mut Self::UdpSocket, remote: SocketAddr, buffer: &[u8]) -> Result<(), nb::Error<<T as embedded_nal::UdpClient>::Error>>}
}
impl<'a, T: TcpStack> TcpStack for SharedNal<'a, T> {
type TcpSocket = T::TcpSocket;
type Error = T::Error;
forward! {open() -> Result<Self::TcpSocket, Self::Error>}
forward! {connect(socket: Self::TcpSocket, address: SocketAddr) -> Result<Self::TcpSocket, Self::Error>}
forward! {is_connected(socket: &Self::TcpSocket) -> Result<bool, Self::Error>}
forward! {write(socket: &mut Self::TcpSocket, buffer: &[u8]) -> nb::Result<usize, Self::Error>}
forward! {read(socket: &mut Self::TcpSocket, buffer: &mut [u8]) -> nb::Result<usize, Self::Error>}
forward! {close(socket: Self::TcpSocket) -> Result<(), Self::Error>}
}
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#39 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACC5O6QNUS24LG7VBEZGMYDS2M65JANCNFSM4VPOPDZQ>
.
|
|
A point worth considering in this, would also be that |
|
I think we've come to the realization that any thread-safety provided by Thoughts? |
|
Having looked a little deeper into I'm not convinced that it has to be in it's own crate though, because it is such a common requirement and |
As far as I'm concerned, that's out-of-scope for the discussion in this thread. We do need something like |
|
Am I understanding the consensus to be
|
I don't understand the reason for immutable self references that clearly do mutate the state of self or require exclusive access to the underlying implementation or bus. I think this design decision only causes hacks to circumvent it, unnecessary overhead in the driver implementation (which is the opposite of 'zero cost'/'you only pay for what you need') and does not reflect the strategy of the rust ecosystem, such as embedded-hal, std-Read, std-Write, tokio-AsyncRead or tokio-AsyncWrite.
I don't follow the argument of @thejpster here as it seems more of an special case scenario and could be solved otherwise by implementing
Cloneon what seems to be an FD or raw-pointer(?) internally.Feel free to convince me otherwise.
We stumbled over this in a discussion here where @jonahd-g (thanky you!) proposes an implementation of embedded-nal for the w5500 driver.
The text was updated successfully, but these errors were encountered: