-
Notifications
You must be signed in to change notification settings - Fork 18
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
Restructure and implement embedded-nal UDP traits #26
Conversation
@kellerkindt what it takes to get it merged, do you see any problems in this approach that you do not like? |
Wow, thanks for your contribution! I'll try to review it on the weekend. I'll be more than happy to merge this - although having to depend on an unreleased version of |
@kellerkindt Yes it is a bummer. Release is pending an update to the TCP trait(s). |
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.
One thing that does not seem to be possible anymore, is to store the W5500
/InactiveDevice
in a struct and to activate it for a short duration as needed?
Before, I would write something like
pub struct SomePlatformDevices {
w5500: W5500<..>,
...
}
impl SomePlatformDevices {
pub fn receive_udp(
&mut self,
buffer: &mut [u8],
) -> Result<Option<(IpAddress, u16, usize)>, TransferError<SpiError, W5500CsError>> {
let mut active = self.w5500.activate(&mut self.spi)?;
let active = &mut active;
let socket = self
.network_udp
.as_ref()
.ok_or(TransferError::SpiError(spi::Error::ModeFault))?;
(active, socket).receive(buffer)
}
}
In the new version:
pub struct SomePlatformDevices {
w5500: InactiveDevice<..>,
...
}
impl SomePlatformDevices {
pub fn receive_udp(
&mut self,
buffer: &mut [u8],
) -> Result<Option<(IpAddress, u16, usize)>, TransferError<SpiError, W5500CsError>> {
let mut active = self.w5500.activate(&mut self.spi)?; // ERROR: this takes ownership/moves w5500
...
}
}
Am I approaching this wrong?
Yeah, this does make that a bit more tricky. I think you could apply the Option pattern here fairly easily though. pub struct SomePlatformDevices {
w5500: Option<InactiveDevice<..>>,
...
}
impl SomePlatformDevices {
pub fn receive_udp(
&mut self,
buffer: &mut [u8],
) -> Result<Option<(IpAddress, u16, usize)>, TransferError<SpiError, W5500CsError>> {
if let Some(w5500) = self.w5500.take() {
let mut active = w5500.activate(&mut self.spi)?;
// do something ...
self.w5500 = Some(w5500.deactivate())
}
}
} |
@kellerkindt we're now depending on a released version of embedded-nal. |
Uh-oh, looks like there were some upstream changes I didn't take into account. Will fix. |
All fixed. |
Looking forward to see it in crates.io, good stuff! |
@kellerkindt are there any issues holding this back from being merged? |
I also voice my interest to get it merged. |
I don't see any issues at first glance and I also want this to be merged, but I also do not want to merge such a big change without testing/playing around with it myself (I plan to upgrade a small personal project). And at the moment, I do not have the time at hand to do so (I assume I`ll need one or two evenings). I hope I will be able to do so the week before Christmas or before new year. Sorry! I am really grateful for all your effort! |
Thanks @kellerkindt . I'd be interested in becoming a maintainer in the future if you're open to that, that way the project has a bit of redundancy. Other things I'd like to work on after this is merged:
Re: merging a big change, you could merge it and release as a beta, i.e. |
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 just ported over a small project of mine so that it uses this PR. I had to make some changes to be able to use it.
I can see the appeal of having definite states, but using it feels a bit clumsy. It also did not find one of my errors while porting it over: I failed to re-bind the UdpSocket
after sending data. Also the fact that the device instance is moved in and out of the Option
(self.w5500
in the code snippets below) just screams for someone to forget to put it back under some edge case scenario. Moving the InactiveDevice
in and out from the option, calling .activate(SpiRef(&mut self.spi)).into_interface()
and interface.release().deactivate().0
is also not a perfect experience, especially if you remember that internally it is moved in and out of the RefCell
and additional overhead for the runtime borrow checks is caused.
If I see this correctly, the RefCell
and into_interface()
/release()
calls are only required because the function calls in UdpClient take a shared reference instead of an exclusive/mut reference? Can someone explain me the reason of it? Because I don't get it. No trait of embedded-hal does it (see Spi/FullDuplex for example). People seem to circumvent it anyways?
As an additional note, the chip version assertion does not cover all scenarios? I am working with at least one chip, for which the check does not work at all, so I had to add a way to disable the check.
Below are some snippets of the code that uses this PR - please excuse my (now) poorly chosen fn-signatures, I only replaced the body parts.
...
w5500: {
let mut w5500_cs = gpiob.pb1.into_push_pull_output(&mut gpiob.crl);
w5500_cs.set_low_infallible(); // deselect
w5500_reset.set_high_infallible(); // request reset
delay.delay_ms(250_u16); // allow boot
w5500_reset.set_low_infallible();
let interface = Interface::setup(
SpiRef(&mut spi),
w5500_cs,
MacAddress::new(0x02, 0x00, 0x00, 0x00, 0x00, 0x00),
IpAddress::from(NetworkConfiguration::DEFAULT_IP.octets()),
);
if let Ok(interface) = interface {
Some(interface.release().deactivate().0)
} else {
None // no way to recover
}
},
...
pub fn init_network(&mut self) -> Result<(), SpiError> {
if let Some(device) = self.w5500.take() {
let mut active = device.activate(SpiRef(&mut self.spi)).into_interface();
if let Some(socket) = self.network_udp.take() {
let _ = active.close(socket);
}
let (bus, config) = active.release().release();
// reset the network chip (timings are really generous)
self.w5500_reset.set_low_infallible();
self.system.delay.delay_ms(250_u16); // Datasheet 5.5.1 says 500_us to 1_ms (?)
self.w5500_reset.set_high_infallible();
self.system.delay.delay_ms(250_u16); // Datasheet says read RDY pin, the network chip needs some time to boot!
self.system.delay.delay_ms(100_u16);
let mut device = match UninitializedDevice::new(bus).initialize_advanced(
self.network_config.mac,
self.network_config.ip,
self.network_config.gateway,
self.network_config.subnet,
w5500::Mode {
on_wake_on_lan: OnWakeOnLan::Ignore,
on_ping_request: OnPingRequest::Respond,
connection_type: ConnectionType::Ethernet,
arp_responses: ArpResponses::Cache,
},
) {
Ok(device) => device,
Err(_) => panic!("lul"), // no way to recover
};
self.system.delay.delay_ms(100_u16);
// init the UdpSocket
if let Some(socket) = device.take_socket() {
let mut udp = UdpSocket::new(socket);
let mut interface = device.into_interface();
interface.bind(&mut udp, SOCKET_UDP_PORT);
self.network_udp = Some(udp);
self.w5500 = Some(interface.release().deactivate().0);
} else {
self.w5500 = Some(device.deactivate().0);
}
}
Ok(())
}
...
pub fn receive_udp(
&mut self,
buffer: &mut [u8],
) -> Result<Option<(IpAddress, u16, usize)>, SpiError> {
if let Some(socket) = self.network_udp.as_mut() {
if let Some(device) = self.w5500.take() {
let mut interface = device.activate(SpiRef(&mut self.spi)).into_interface();
let result: nb::Result<_, _> = interface.receive(socket, buffer);
self.w5500 = Some(interface.release().deactivate().0);
return match result {
Ok((_, SocketAddr::V6(_))) => unreachable!(),
Ok((len, SocketAddr::V4(v4))) => Ok(Some((*v4.ip(), v4.port(), len))),
Err(...) => ...
};
}
}
Ok(None)
}
...
pub fn send_udp(&mut self, host: &IpAddress, port: u16, data: &[u8]) -> Result<(), SpiError> {
if let Some(socket) = self.network_udp.as_mut() {
if let Some(device) = self.w5500.take() {
let mut interface = device.activate(SpiRef(&mut self.spi)).into_interface();
let result = block!(interface.send_to(
socket,
SocketAddr::V4(SocketAddrV4::new(*host, port)),
data
));
interface.bind(socket, SOCKET_UDP_PORT);
self.w5500 = Some(interface.release().deactivate().0);
return result.map_err(|e| ...);
}
}
Ok(())
}
...
/// Helper to circumvent Spi-Ownership
#[derive(Debug)]
pub struct SpiRef<'a, Spi: FullDuplex<u8>>(&'a mut Spi);
impl<'a, Spi: FullDuplex<u8>> FullDuplex<u8> for SpiRef<'a, Spi> {
type Error = Spi::Error;
fn read(&mut self) -> nb::Result<u8, Self::Error> {
self.0.read()
}
fn send(&mut self, word: u8) -> nb::Result<(), Self::Error> {
self.0.send(word)
}
}
register: block + 1, | ||
tx_buffer: block + 2, | ||
rx_buffer: block + 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.
Unnecessary memory consumption? Can be computed in the getter fns
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'm not certain. This should only consume 3 bytes, but I have no idea how much memory would be consumed by three separate functions dedicated to adding three different integers (plus the extra time needed to compute upon every use). Thoughts?
fn refresh<SpiBus: ActiveBus>(&mut self, bus: &mut SpiBus) -> Result<(), SpiBus::Error> { | ||
if !self.is_setup { | ||
Self::write_settings(bus, &mut self.current, &self.settings)?; | ||
self.is_setup = true; |
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 only writes the settings once? Meaning, it compares the user settings with current
which is always HostConfig::default()
, but still stores two instances and the flag in memory?
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.
Correct, it'll only write once. This is the intent, to keep a consistent interface between Manual and DHCP. DHCP needs to be periodically renegotiated. Manual doesn't need to, so after the initial setup it does nothing. I suppose this might be improved, but this is a private API, and DHCP isn't implemented yet, so I'd have no issues with merging as is and then re-considering once I start on DHCP. In fact I expect that I'll need to.
self.initialize_with_host(host, mode_options) | ||
} | ||
|
||
fn initialize_with_host<HostImpl: Host>( |
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 self.bus
is lost on an error here, so there is no way to recover from a temporary issue
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 for Interface::setup
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 agree, however I'm not certain that's a huge issue in this particular case. UninitializedDevice can be re-created at basically no cost, it has nothing without being initialized.
} | ||
} | ||
|
||
pub fn reset(mut self) -> Result<UninitializedDevice<SpiBus>, ResetError<SpiBus::Error>> { |
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.
self
is lost on an error here, so there is no way to recover from a temporary issue
Thanks for the thorough review. I'll have some time to address your feedback in the new year. Re: the usage of internal mutability, I wasn't a huge fan either. It is a justifiable design decision though. rust-embedded-community/embedded-nal#4 (comment) The traits mandate immutable references, which makes a difference in implementation style impossible. If you want to take the design choice war to the NAL repo, I'll have your back 😉 |
@kellerkindt I think we're close to a win here! Once it's merged I'll update this PR and review your feedback. |
I have no explanation for that, I pulled that value directly from the datasheet. Can you determine what the value is, or track down a datasheet that mentions a different number?
|
Note-to-self, I've reviewed and addressed all of the feedback except for this paragraph:
I will consider the issues brought up further and come up with a solution/response. |
@kellerkindt I've spent several hours tweaking and thinking, and I can see a couple of simplifications I could make here.
Also, can you provide some further clarification on this comment?
I don't understand what you mean here... why does a socket need to re-bound? |
I've gone ahead and made the changes I mentioned. The only outstanding issues AFAIK are
|
…traits to allow users to use shared-bus
Rebased on to the upstream master branch. @kellerkindt just waiting on those details mentioned in my previous post. |
|
||
const WRITE_MODE_MASK: u8 = 0b00000_1_00; | ||
|
||
// TODO This name is not ideal, should be renamed to VDM |
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.
What stands VDM for?
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.
const FIXED_DATA_LENGTH_MODE_4: u8 = 0b000000_11; | ||
|
||
// TODO This name is not ideal, should be renamed to FDM | ||
pub struct ThreeWire<Spi: Transfer<u8> + Write<u8>> { |
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.
What stands FDM for?
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.
Fixed length Data Mode (see screenshot of datasheet above).
/// Transfers a frame with an arbitrary data length in FDM | ||
/// | ||
/// This is done by passing chunks of fixed length 4, 2, or 1. For example if a frame looks like this: | ||
/// | ||
/// (address 23) 0xF0 0xAB 0x83 0xB2 0x44 0x2C 0xAA | ||
/// | ||
/// This will be sent as separate frames in the chunks | ||
/// | ||
/// (address 23) 0xF0 0xAB 0x83 0xB2 | ||
/// (address 27) 44 2C | ||
/// (address 29) AA |
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.
What is the advantage of this behavior?
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.
These two modes are for when you either have or do not have a CS pin. You can connect with only three wires (MISO, MOSI, CLK), but you have to use FDM.
use crate::socket::Socket; | ||
use crate::uninitialized_device::UninitializedDevice; | ||
|
||
pub struct Device<SpiBus: Bus, HostImpl: Host> { |
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.
Shouldnt this be called something like W5500
instead of generically calling it Device
?
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 felt that was kind of repetitive, considering the crate is already called w5500
. w5500::W5500
. IMO, I'm willing to discuss renaming.
Sorry, I missed that. Thanks for the reminder!
I am not entirely sure what my issue was, but I think it was along the lines of receiving data and wanting to send a response, which failed (without further clarification) until I bound the (same) socket as UdpSocket (again). I appreciate your effort - I really do - but I`ll be unable to test your most recent changes in the next few days. I still would like to have an additional way to not own the bus by the Device / borrow it for when I am using it to talk to the W5500 (so I can do some tricks with the scope and lifetimes), but I'll play around with that on my own, after this PR is merged. |
Yes
Hmm, I'll try to write a UDP server and see if I can replicate the issue. |
Finally got back around to this. I am able to replicate the issue around needing to re-bind, using this code: let device = UninitializedDevice::new(FourWire::new(spi, cs)).initialize_manual(MacAddress::new(0, 1, 2, 3, 4, 5), Ipv4Addr::new(192, 168, 86, 79), Mode::default());
if let Ok(mut device) = device {
let socket = device.socket();
if let Ok(mut socket) = socket {
// Binding inside loop works
device.bind(&mut socket, 8000);
loop {
let mut buf = [0u8; 16];
if let Ok((size, addr)) = block!(device.receive(&mut socket, &mut buf)) {
ufmt::uwriteln!(&mut serial, "received {} {} {} {} {} {}", buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]).unwrap();
device.send_to(&mut socket, addr, &[104, 101, 108, 108, 111, 10]);
}
}
}
} Since I'm able to replicate reliably I should be able to figure out what's causing the issue. |
The fix turned out to be very simple! Just needed to re-open the socket after executing |
f8008e0
to
2b82c7f
Compare
pub use self::four_wire::FourWire; | ||
pub use self::three_wire::ThreeWire; |
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.
How to access FourWire/ThreeWire Error when not public?
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 what you mean, bus
is public.
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 wasn't public before. Otherwise one could not work with the FourWireError type?
pub struct UninitializedDevice<SpiBus: Bus> { | ||
bus: SpiBus, | ||
} |
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.
How to restore after deactivate
from HostImpl without resetting / loosing device buffer?
Locally I added
pub fn restore_from_host<HostImpl: Host>(mut self, host: HostImpl) -> Device<SpiBus, HostImpl> {
Device::new(self.bus, host)
}
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 can be achieved by instead using initialize_with_host
. I'll open a PR to make that function public.
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.
But initialize_with_host
resets the device.
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 added InactiveDevice
for that reason - or do you have better solution?
is_setup: bool, | ||
settings: HostConfig, | ||
current: HostConfig, |
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 quite a fat struct
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.
What do you mean?
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 Device<Manual>
is about 38 bytes large in memory:
- ~0 bytes for the SPI-Bus (depending on the optimization)
- 37 bytes for HostImpl (for
Manual
)- 1 byte for
is_setup
(maybe we could eliminate this with a generictype andPhantomData
, e.g.Manual<Setup>
andManual<NotSetup>
) - 36 bytes because of 2x
HostConfig
- 6 bytes for
mac
- 4 bytes for
ip
- 4 bytes for
gateway
- 4 bytes for
subnet
- 6 bytes for
- 1 byte for
- 1 byte for the sockets
What I mean is, that "just for" the feature of not having to always send the whole config in Host::refresh
all Device
instances consume 18 additional bytes in memory. Assuming that everyone is okay with spending 18 bytes for having the current configuration cached in the first place. In my personal projects I neither need the current state nor the "shall be" state, which means the Device
instance is - for me - 36 bytes / 3600% larger than needed.
What I just noticed, If we were to pass the "shall value" HostConfig
into Host::refresh
we could free up 18 bytes. Because of the trait we would probably require a associated type? Like:
pub trait Host {
type Config;
/// Gets (if necessary) and sets the host settings on the chip
fn refresh<SpiBus: Bus>(&mut self, bus: &mut SpiBus, new_config: &Self::Config) -> Result<(), SpiBus::Error>;
//...
}
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 raise some good points. I'll work on cutting this down.
woohoo, thanks for merging! I'll get on your latest round of feedback asap (probably tomorrow morning). |
This has been a long time in the works (over a year), but it's finally working and ready for a review. This PR majorly restructures the driver into a state machine design so that only valid actions can be taken. It abstracts the SPI connection into a Bus trait, with implementations for a three-wire and a four-wire connection. It also abstracts the host config into a Host trait, with implementations for Manual and DHCP (incomplete). And importantly, it implements the embedded-nal UDP traits.
This is depending on an un-released rev of embedded-nal, since the latest UDP traits have not been released yet. If this is merged before that is released, I'll create another PR to depend on the new version.