Take ownership of SPI/CS?

[jonahbron]

What do you think about changing the interface so that the W5500 and ActiveW5500 structs take ownership of the SPI/CS chips instead of accepting mutable references to them? Taking ownership of them seems like the more obvious approach to me, but maybe there are factors to use &mut instead that I'm not aware of.

[kellerkindt]

I am still not sure which of those paths I prefer. On the one hand, when having references, dropping a W5500 or ActiveW5500 instance will immediately make the OutpuPin / SPI available again for other use cases, without reminding oneself to manually retrieve it on destruction. On the other hand, the lifetime definitions make the code a lot less readable. Taking the ownership seems to be the common ideology?

Taking the ownershhip of the OutputPin would most probably fine, because it is highly unlikely that anyone will use this pin for something else. This would eliminate quite a bit of lifetime declarations. Taking the ownership of an SPI instance is a lot less reasonable because reusing it it is highly probable. I would prefer one common strategy for both structs.

What do you think?

[jonahbron]

The syntax of the language poses ownership as the default path (ex: cs vs &mut cs). In addition, passing the ownership around seems to me to be a more explicit approach, vs implicit, i.e. explicitly invoking a release() method to get the SPI instance back, instead of implicitly allowing the reference to go out of scope. I agree that it's unlikely that the chip-select pin would need to be used elsewhere in a given program. And while you're right that the SPI interface is likely to be shared, it's not difficult to explicitly pull ownership back out. Here's the interface that I envision:

let cs = ...;
let spi = ...
let inactive_ethernet = W5500::with_initialization(cs, ...);
let ethernet = inactive_ethernet.activate(spi);// inactive_ethernet is owned by ethernet
// do something on the network
let (inactive_ethernet, spi) = ethernet.deactivate(spi);// or `release`
// spi available for something else
let cs = inactive_ethernet.release();
// cs available for something else

In this example, even the W5500 instance is owned by ActiveW5500. With this approach, things are more explicit, and an argument could be made that additional code simply isn't worth it. However I feel it's more idiomatically aligned with how Rust is designed.

Just my $0.02. Take it with a grain of salt, I've started learning rust about 4 weeks ago.

[jonahbron]

I've put together my vision as an informal state machine diagram.

https://docs.google.com/drawings/d/1Yq0XQ_DKhvvv8n4GElfYEy7dBVHKeKAfOQJQfE9o1cA/edit

The lines show the methods that can be called from each state to transition the state. For example, calling deactivate on a W5500 gives you a InitializedW5500. I think modeling the driver as a state machine is very complementary to Rust's ownership system. I also think I have a good idea on how to use the compiler to enforce one-at-a-time use of sockets, instead of needing to check at run-time. Here is a rough outline of the structs and their state-transitioning methods (of course they'd also have useful methods for doing things).

struct Bus {
    cs: OutputPin,
}

struct ActiveBus {
    spi: FullDuplex<u8>,
    bus: Bus,
}

struct UninitializedW5500 {
    bus: ActiveBus,
}
impl UninitializedW5500 {
    fn initialize(self, WakeOnLan, OnPingRequest, ...) -> (W5500, Socket0, Socket1, Socket2, Socket3, Socket4, Socket5, Socket7, Socket8) {
        // config chip
        (W5500 { self.bus }, Socket0 {}, Socket1 {}, Socket2 {}, Socket3 {}, Socket4 {}, Socket5 {}, Socket6 {}, Socket7 {})
    }
    fn deactivate(self) -> FullDuplex {
        self.bus.spi
    }
}

struct W5500 {
    bus: ActiveBus,
}
impl W5500 {
    fn reset(self) -> UninitializedW5500 {
        // reset chip
        UninitializedW5500 { bus: self.bus }
    }
    fn deactivate(self) -> (InactiveW5500, FullDuplex) {
        (InactiveW5500 { bus: self.bus.bus }, self.bus.spi)
    }
    fn take_udp_socket(self, socket: Socket, port: u16) -> UdpSocket {
        // set up socket for UDP mode
        UdpSocket { socket, chip: self }
    }
}

struct InactiveW5500 {
    bus: Bus
}
impl InactiveW5500 {
    fn activate(self, spi: FullDuplex) -> W5500 {
        W5500 { bus: ActiveBus { spi, bus: self.bus } }
    }
}

struct UdpSocket {
    socket: Socket,
    chip: W5500,
}
impl UdpSocket {
    fn release(self) -> (W5500, Socket) {
        // reset socket
        (self.chip, self.socket)
    }

    fn deactivate(self) -> (W5500, InactiveUdpSocket) {
        (self.chip, InactiveUdpSocket { socke})
    }
}

struct InactiveUdpSocket {
    socket: Socket,
}
impl InactiveUdpSocket {
    fn activate(self, chip: W5500) -> UdpSocket {
        UdpSocket { socket: self.socket, chip }
    }
}
// TcpSocket
// InactiveTcpSocket

[kellerkindt]

That sounds quite neat.
In addition to your code snippet (I guess you meant something like that?), with the following, the socket-id handling would also be moved from runtime to compile time:

pub struct Socket0;
pub struct Socket1;
pub struct Socket2;
pub struct Socket3;
pub struct Socket4;
pub struct Socket5;
pub struct Socket6;
pub struct Socket7;

pub trait Socket {
    const ID: u8;
}

impl Socket for Socket0 {
    const ID: u8 = 0;
}
impl Socket for Socket1 {
    const ID: u8 = 1;
}
// ...

// maybe Udp instead of UdpSocket when using generics?
// The full name would then be for example Udp<Socket0>,
// instead of UdpSocket<Socket0>, so no 'Socket' repetition
struct Udp<S: Socket> {
    socket: S,
// ...
}

Two notes:

• I wouldn't call it take_udp_socket, maybe try_to_udp_socket or something like that, because nothing is actually taken from the W5500 instance
• Maybe the W5500::reset(self) -> InactiveW5500 should take all spawned Sockets back? Otherwise through multiple initialize -> reset -> initialize -> reset -> ... rounds, eight additional sockets are spawned for each call of initialize, but only the most recent set is actually valid.

All in all, I quite like this approach

[jonahbron]

That sounds quite neat.

Thanks, I'm glad you think so 😄

Yes, I totally agree on your socket ID idea.

I wouldn't call it take_udp_socket, maybe try_to_udp_socket or something like that, because nothing is actually taken from the W5500 instance

Good point. What about open_udp_socket?

Maybe the W5500::reset(self) -> InactiveW5500 should take all spawned Sockets back? Otherwise through multiple initialize -> reset -> initialize -> reset -> ... rounds, eight additional sockets are spawned for each call of initialize, but only the most recent set is actually valid.

Excellent point. You're right if we don't clean up those sockets, they would be invalid. Accepting them as args would be a way to do that. And even though that would be pretty verbose, it wouldn't be necessary for most programs. A basic program might look something like this.

let inactive_w5500 = InactiveW5500::new(spi, cs);
let (w5500, socket0, ..) = inactive_w5500.initialize(/* settings */).unwrap();
let udp_socket = w5500.open_udp_socket(socket0).unwrap();
// send/receive on udp_socket

The only problem with this that I can think of so far is that if you had two W5500s and two driver instances, you could interchange their Sockets to bypass the compile-time ownership checking. I need to think some more about how to get the compiler to only accept the instances it instantiated itself.

[jonahbron]

Idea: if the W5500 instantiates a symbol struct that it owns, it could instantiate each socket with a ref to that symbol. That would require

1. sockets go out of scope before the W5500 is deinitialized, and
2. it could be used in a simple equality check at run-time to ensure that a socket does indeed belong to that particular W5500 instance.

It wouldn't be compile-time, but at least it would be very fast. The only way I can think of to make the sockets unique per driver at compile time is to get tricky with generics, so that the compiler treats every W5500 instance as a different type.

[jonahbron]

See my progress here: https://github.com/jonahbron/w5500/tree/restructure

[jonahbron]

@kellerkindt Just some updates on my progress thus far. I've created a trait called Bus. It represents the interface between the driver implementation and the chip itself over SPI. There are two implementations: ThreeWire and FourWire. FourWire is the default, and it uses the CS and operates in VDM. ThreeWire is optional, and instead operates in FDM, and splits any data phase not of length 1, 2, or 4 into its constituent chunks and sends them one at a time. Pretty neat I think. This approach allows the compiler to only include the bus option(s) used by the program.

There are also three other main structs: InactiveW5500, UninitializedW5500, and W5500. Their relationship is still as was shown in that finite state diagram. W5500 can be reset or unactivated, InactiveW5500 can be activated, and UninitializedW550 can be initialized.

The initialize method accepts a struct of Settings that contains the four mode options you made.

I've realized that there needs to be a trait for NetworkSettings. It could have two implementations, Manual and Dhcp. The W5500 should then receive one or the other. This allows the compiler to only include the DHCP implementation if the program needs it.

Also still need to work on socket management and the UDP implementation.

[jonahbron]

I've come to the belief that it is impossible to guarantee the consistency of the sockets at compile-time. The only way for the compiler to ensure that the socket it receives is a socket it created is for every driver instance to have a unique type. But this is not possible because the number of driver instances isn't necessarily known at compile time. The user could theoretically run a loop of indeterminate length, producing unique driver instances not known at compile-time.

Thus I've pivoted the concept for Socket management. As of the current state of the branch, the W5500 struct now holds ownership of all 8 sockets, and will produce mutable references upon request. These mutable references can be passed to a new UdpSocket. I believe this way, the borrow checker should be able to guarantee that the socket references leave scope before the W5500 is uninitialized or leaves scope. I haven't actually written a program that uses the library yet, so I'm not 100% sure this will work properly in practice. The library does compile though. If you have some time to review, I'd greatly appreciate it.

[kellerkindt]

Woah, quite a lot of things you fixed in your restructure branch.
Just scrolled through, and so far, I like it quite a lot :)
I am not sure when I will have the time to test it out on my own.
But hopefully, I will find the time next weekend. Sorry!

[jonahbron]

@kellerkindt I took a break for about a week and a half, but I'm back at it. At this point, the code is to the point where it can actually initialize a socket for use in UDP mode. Next thing to work on is the ability to actually send UDP packets.

Again this compiles, but I have not yet tested on hardware. I'll probably do that after packet send/receive is theoretically working.

[jonahbron]

@kellerkindt Would you mind explaining this code?

        let receive_size = loop {
             let s0 = w5500.read_u16(socket.at(SocketRegister::RxReceivedSize))?;
             let s1 = w5500.read_u16(socket.at(SocketRegister::RxReceivedSize))?;
             if s0 == s1 {
                 break s0 as usize;
             }
         };

It appears that it keeps checking the received size until it stops changing. Is that explicitly the design of the chip, that you don't know exactly when the packet has completed until it stops coming Is it possible for the packet to get delayed such that it thinks the packet is fully loaded but isn't? I've been poring over the datasheet and see no mention of that behavior. I'd like to make sure that I fully understand it before I move it over.

Edit: I see this is also the behavior of the Arduino Ethernet library

[kellerkindt]

In short, the doc(?) mentioned that when two reads of the size register return the same value, a new message / chunk has been received. If the two reads are not equal, it is still receiving data.

[kellerkindt]

@kellerkindt I took a break for about a week and a half, but I'm back at it. At this point, the code is to the point where it can actually initialize a socket for use in UDP mode. Next thing to work on is the ability to actually send UDP packets.

Again this compiles, but I have not yet tested on hardware. I'll probably do that after packet send/receive is theoretically working.

Great news. Take your time :)

[jonahbron]

In short, the doc(?) mentioned that when two reads of the size register return the same value, a new message / chunk has been received. If the two reads are not equal, it is still receiving data.

Can you refer me to that documentation? I was unable to find it in the datasheet, perhaps it's somewhere else?

        // // |<-- read_pointer                                read_pointer + received_size -->|
        // // |Destination IP Address | Destination Port | Byte Size of DATA | Actual DATA ... |
        // // |   --- 4 Bytes ---     |  --- 2 Bytes --- |  --- 2 Bytes ---  |      ....       |

I'm also encountering some confusion here. I see no info in the datasheet concerning the structure of the socket RX buffer. This structure described in your code comments does not line up with what the docs say about an actual UDP packet header:

https://erg.abdn.ac.uk/users/gorry/course/inet-pages/udp.html

My only guess is that the W5500 does some sort of parsing on the UDP packet itself (assumedly verifying the checksum, since it's not present in the comment). Can you refer me to the documentation that describes this as well?

[jonahbron]

@kellerkindt Never mind about the two-reads of the size thing, my datasheet must have been out-of-date. Found one on the WizNet website that mentions that necessity.

http://wizwiki.net/wiki/lib/exe/fetch.php?media=products:w5500:w5500_ds_v109e.pdf page 55 "Sn_RX_RSR"

I'm still unable to find any mention of the structure of the socket buffer however.

[jonahbron]

All righty, I found it. A bit of googling revealed this info is missing from the W5500 docs, but is present in the W5200 docs.

https://forum.wiznet.io/t/topic/691

W5200 datasheet, page 5.2.2
http://wiznethome.cafe24.com/wp-content/uploads/wiznethome/Chip/W5200/Documents/W5200_DS_V140E.pdf

[jonahbron]

@kellerkindt Tell me what you think of this, but I've added a new struct called UdpPacket. It's purpose is to parse and read a packet upon receipt. It's created by UdpSocket when you call receive(). Instead of passing an array reference in, you keep calling read() until the entire packet has been read. I want to make this an iterator too so that the program can simply loop over the packet. This makes it so the program doesn't need to allocate a fixed amount of space to fit the packet.

I think we should still keep a method called read_all or something so if the user wants to load the whole packet into a fixed buffer space, they can.

[jonahbron]

UDP packet receipt is fleshed out. Not tested, but I'm going to move on to packet sending before testing.

[jonahbron]

@kellerkindt I modified the design, there's now an IncomingPacket and an OutgoingPacket. The both hold the socket (and the SPI peripheral) so that nothing else can be done until the packet is read or sent, respectively. Give it a peek when you have time. I'll try to run an actual test this weekend.

[jonahbron]

Upon attempting to integrate the library into an application, I've finally verified there's no way to do the kind of socket enforcement I've been trying to do. Will need to come up with a more run-time approach 😢

       52:28  	rustc       	error     	cannot borrow `w5500` as mutable, as it is not declared as mutable
             	            	          	
             	            	          	cannot borrow as mutable

[jonahbron]

@kellerkindt Okay I was able to get a simple packet-sending script to compile after some alterations to the driver.

    let uninitialized_w5500 = UninitializedW5500::new(FourWire::new(cs).activate(spi));
    let w5500 = uninitialized_w5500.initialize_manual(MacAddress::new(0, 1, 2, 3, 4, 5), IpAddress::new(192, 168, 86, 30), Mode::default());// handle error
    if let Ok((w5500, (socket, ..))) = w5500 {
        let udp_socket = w5500.open_udp_socket(8000, socket);
        if let Ok(udp_socket) = udp_socket {
            let packet = udp_socket.send(IpAddress::new(192, 168, 86, 35), 4000);
            if let Ok(mut packet) = packet {
                packet.write(&mut [104, 101, 108, 108, 111]);// ASCII "hello"
                packet.send();
            }
        }
    }

The good news is that this compiles. The bad news, is that I'm now getting the same error as described in #6 . The gcc-linker step fails. It will succeed if I remove the packet.send invocation and the refresh call in UninitializedW5500. But without at least the packet.send, this program is useless. I really hope someone can figure out where this error is coming from.

[jonahbron]

All right after adding compiler-builtins to my project, I can now compile and upload a program to the Arduino. With the program I've been able to read/write from/to some registers of the chip. However, I've run into some trouble with the process of actually sending a UDP packet. I got a dump of the Socket0 register to show what's being written correctly and what's not.

Sn_MR           2       UDP, correct
Sn_CR           0       is set to 0x20 but immediately gets cleared, correct
Sn_IR           0       no status, correct
Sn_SR           0       not clear what status should be on UDP send. UNSURE
Sn_PORT0        31
Sn_PORT1        64      confirmed port 8000, expected
Sn_DHAR0        255     can be anything, as long as not using SEND_MAC command
Sn_DHAR1        255     ditto
Sn_DHAR2        255     ditto
Sn_DHAR3        255     ditto
Sn_DHAR4        255     ditto
Sn_DHAR5        255     ditto
Sn_DIPR0        0       should be 192, INCORRECT
Sn_DIPR1        0       should be 168, INCORRECT
Sn_DIPR2        0       should be 86, INCORRECT
Sn_DIPR3        0       should be 21, INCORRECT
Sn_DPORT0       0       should be 15, INCORRECT
Sn_DPORT1       0       should be 160, INCORRECT
Sn_MSSR0        0       for TCP, not needed, correct
Sn_MSSR1        0       for TCP, not needed, correct
Reserved        0
Sn_TOS          0       should be not needed, correct
Sn_TTL          128     should be not needed, correct
Reserved        0
Reserved        0
Reserved        0
Reserved        0
Reserved        0
Reserved        0
Reserved        0
Sn_RXBUF_SIZE   2       default of 2KB, correct
Sn_TXBUF_SIZE   2       default of 2KB, correct
Sn_TX_FSR0      8
Sn_TX_FSR1      0
Sn_TX_RD0       0
Sn_TX_RD1       0
Sn_TX_WR0       0
Sn_TX_WR1       0       should be equal to the length of the payload, INCORRECT
Sn_RX_RSR0      0
Sn_RX_RSR1      0
Sn_RX_RD0       0
Sn_RX_RD1       0
Sn_RX_WR0       0
Sn_RX_WR1       0
Sn_IMR          255     at reset value, seems to be correct
Sn_FRAG0        64
Sn_FRAG1        0       seems correct
Sn_KPALVTR      0       seems correct

What's very odd is that no matter what I do, writing to anything past the source port (port 0) doesn't seem to stick. That dump of the register above is after the program writes several registers including destination IP (DIPR). But they still read as though they were never written to.

[kellerkindt]

Sorry for the lack of responses 😢
How are you doing? I am a bit at a loss, on my stm32f103 it works flawlessly (well, I am only receiving and sending a few UDP packets...)

[jonahbron]

I got sidetracked for a while working on an issue on a lower layer. I should be able to pick this back up again and try to figure it out.

[kellerkindt]

Hey, great news! But don't stress yourself. I'll be happy to accept your PRs when they are ready!

[jonahbron]

Excited to announce that now that AVR-Rust has been upstreamed, I can finally (once again) compile this library to my target platform. I will be resuming development.

[jonahbron]

Finally have everything back up and running. I can talk to the chip again (in my restructure branch), and can read/write registers. However I'm running into a bit of a wall. No matter what I do, sending the Open command to the socket doesn't change the status register to SOCK_UDP. Having trouble figuring out why.

https://github.com/jonahbron/w5500/blob/restructure/src/udp/mod.rs#L29

[ryan-summers]

Super late to this party:

Just an FYI, but from an embedded rust perspective, it's very idiomatic for drivers to take ownership of the communication interface (e.g. SPI in this case).

There are possibilities where SPI may be used by multiple devices, but there's actually other crates that facilitate this, so each driver can still own the SPI peripheral. Ref: https://crates.io/crates/shared-bus and https://crates.io/crates/shared-bus-rtic

The gist is that these crates provide a "proxy" to the SPI bus, which also implements the associated traits. This allows multiple drivers to "own" the SPI bus at the same time. Those crates facilitate the resource sharing aspect of it.

[jonahbron]

@ryan-summers That's good to know, thank you.

@kellerkindt I've got some great news. After much gnashing of teeth, I found just a few little (ba78c6d) bugs that were preventing my tests from working. I now officially have a test program that can send a UDP packet 🎉

390f87d has the working library version as of this comment, and here is the full test program code:

#![no_std]
#![no_main]

extern crate panic_halt;

use arduino_uno::spi::{Spi, Settings, DataOrder, SerialClockRate};
use arduino_uno::prelude::*;
use embedded_hal::spi;
use nb::block;

use w5500::bus::FourWire;
use w5500::bus::ActiveBus;
use w5500::register;
use w5500::socket::Socket0;
use w5500::socket::Socket;
use w5500::IpAddress;

use w5500::uninitialized_w5500::{UninitializedW5500,InitializeError};
use w5500::MacAddress;
use w5500::Mode;

#[arduino_uno::entry]
fn main() -> ! {
    let dp = arduino_uno::Peripherals::take().unwrap_or_else(|| panic!());
    let mut delay = arduino_uno::Delay::new();
    let mut pins = arduino_uno::Pins::new(dp.PORTB, dp.PORTC, dp.PORTD);

    let mut serial = arduino_uno::Serial::new(
        dp.USART0,
        pins.d0,
        pins.d1.into_output(&mut pins.ddr),
        57600,
    );
    let cs = pins.d10.into_output(&mut pins.ddr);

    let mut spi = arduino_uno::spi::Spi::new(
        dp.SPI,
        pins.d13.into_output(&mut pins.ddr),
        pins.d11.into_output(&mut pins.ddr),
        pins.d12.into_pull_up_input(&mut pins.ddr),
        Settings {
            data_order: DataOrder::MostSignificantFirst,
            clock: SerialClockRate::OscfOver4,
            mode: spi::Mode {
              polarity: spi::Polarity::IdleLow,
              phase: spi::Phase::CaptureOnFirstTransition,
            }
        },
    );

    let mut bus = FourWire::new(cs).activate(spi);

    let uninitialized_w5500 = UninitializedW5500::new(bus);
    let w5500 = uninitialized_w5500.initialize_manual(MacAddress::new(0, 1, 2, 3, 4, 5), IpAddress::new(192, 168, 86, 79), Mode::default());// handle error
    if let Ok((w5500, (socket, ..))) = w5500 {
        let udp_socket = w5500.open_udp_socket(8001, socket);// source port gets set correctly
        if let Ok(mut udp_socket) = udp_socket {
            let packet = udp_socket.send(IpAddress::new(192, 168, 86, 38), 8000);
            if let Ok(mut packet) = packet {

                let wrote = packet.write(&mut [104, 101, 108, 108, 111, 10]);
                if let Ok(()) = wrote {
                    let sent = packet.send();
                    if let Ok(mut udp_socket) = sent {
                        ufmt::uwriteln!(&mut serial, "packet sent\r").unwrap();
                    } else {
                        ufmt::uwriteln!(&mut serial, "packet send failed\r").unwrap();
                    }
                } else {
                        ufmt::uwriteln!(&mut serial, "packet write failed\r").unwrap();
                }
            } else {
                ufmt::uwriteln!(&mut serial, "packet start failed\r").unwrap();
            }
        }
    } else if let Err(InitializeError::ChipNotConnected) = w5500 {
        ufmt::uwriteln!(&mut serial, "chip not connected\r").unwrap();
    } else {
        ufmt::uwriteln!(&mut serial, "not initialized\r").unwrap();
    }
    ufmt::uwriteln!(&mut serial, "DONE\r").unwrap();

    loop {}
}

There is still a bit of clean-up to do on packet sending, but it actually working at all is an exciting threshold. I also need to put together a working test of packet receiving. Following that, I'll need to re-implement some of the newer features introduced upstream.

[andresv]

@jonahbron what are you thoughts about @ryan-summers PR which introduces embedded-nal support?
In the end that would be the most standard way to go (at the moment) and it would be easy to integrate w5500 with other libs if we use some standard trait.

[jonahbron]

I think that's an ideal direction. I haven't heard about embedded-nal until now; I need to read through the docs and see how much it conflicts with my design.

[jonahbron]

Resolved by #26