Skip to content
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

Add ENC424J600 support #134

Closed
wants to merge 14 commits into from
Closed

Add ENC424J600 support #134

wants to merge 14 commits into from

Conversation

occheung
Copy link
Collaborator

This PR adds support to Booster with ENC424J600 as the Ethernet device.
This is implemented by adding feature gates to conditionally compile W5500 and ENC424J600 codes. (i.e. phy_w5500 & phy_enc424j600)
With phy_enc424j600 enabled, Booster uses the factory preprogrammed MAC address of ENC424J600 and ignore the one stored in EEPROM.

Testing

  • Booster with ENC424J600 can send & receive MQTT messages properly, when using firmware built with phy_enc424j600 feature.
  • The expected MAC address is found in network trace and after invoking USB read mac command.
  • The firmware can be compiled with phy_w5500 feature enabled.

Copy link
Member

@jordens jordens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!
I think there is some room for structural improvements that will help with maintaining this code.
And we'll look into moving to raw sockets and smoltcp for the w5500 as well.

Cargo.toml Outdated
Comment on lines 49 to 50
git = "https://github.com/smoltcp-rs/smoltcp.git"
rev = "2080152b"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be blank/crate-versioned since you patch it below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

83380a9 changes this reference to version 0.6.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be smoltcp 0.7.0 and the patch removed?

Copy link
Collaborator Author

@occheung occheung Jan 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be possible after bumping the smoltcp version in enc424j600 crate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enc424j600 should be able to use smoltcp 0.7.0 safely, let's bump that and keep smoltcp up-to-date.

src/settings/mac.rs Outdated Show resolved Hide resolved
src/main.rs Outdated
Comment on lines 466 to 537
match enc424j600.init_dev(&mut delay) {
Ok(_) => {}
Err(_) => {
panic!("ENC424J600 PHY initialization failed");
}
}

enc424j600.read_from_mac(&mut eth_mac_addr).unwrap();

// Init Rx/Tx buffers
enc424j600.init_rxbuf().unwrap();
enc424j600.init_txbuf().unwrap();

let eth_iface = unsafe {
let device = enc424j600::smoltcp_phy::SmoltcpDevice::new(enc424j600);

// TODO: Restore IP config from EEPROM
NET_STORE.ip_addrs[0] = {
let ip = settings.ip().octets();
let subnet = settings.subnet().octets();
net::wire::IpCidr::new(
net::wire::IpAddress::from(net::wire::Ipv4Address::from_bytes(&ip)),
net::wire::IpAddress::from(net::wire::Ipv4Address::from_bytes(
&subnet,
))
.to_prefix_len()
.unwrap(),
)
};

let routes = {
// TODO: Restore gateway config from EEPROM
let gateway =
net::wire::Ipv4Address::from_bytes(&settings.gateway().octets());

let mut routes =
net::iface::Routes::new(&mut NET_STORE.routes_cache[..]);
routes.add_default_ipv4_route(gateway).unwrap();
routes
};

let neighbor_cache =
net::iface::NeighborCache::new(&mut NET_STORE.neighbor_cache[..]);

net::iface::EthernetInterfaceBuilder::new(device)
.ethernet_addr(net::wire::EthernetAddress(eth_mac_addr))
.neighbor_cache(neighbor_cache)
.ip_addrs(&mut NET_STORE.ip_addrs[..])
.routes(routes)
.finalize()
};

let socket_set = unsafe {
let mut sockets =
net::socket::SocketSet::new(&mut NET_STORE.socket_storage[..]);

let mut tcp_socket = {
let tx_buffer =
net::socket::TcpSocketBuffer::new(&mut DATA_STORE.tx_storage[..]);
let rx_buffer =
net::socket::TcpSocketBuffer::new(&mut DATA_STORE.rx_storage[..]);

net::socket::TcpSocket::new(rx_buffer, tx_buffer)
};
tcp_socket.set_keep_alive(Some(net::time::Duration::from_millis(1000)));

let _handle = sockets.add(tcp_socket);
sockets
};

NetworkStack::new(eth_iface, socket_set)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this go into a different module together with the static's above (https://github.com/quartiq/booster/pull/134/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR167-R198)?

I think we have some code in quartiq/stabilizer#230 that looks like it might be similar.

Copy link
Contributor

@HarryMakes HarryMakes Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

15f0964. Does it do what you want?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jordens Stabilizer has moved all hardware init code to a single configuration.rs such that main.rs only needs to call hardware::setup() to return a set of ready-to-use hardware interfaces. Should Booster do the same, and if so, would it be better to do that, along with 15f0964 (splitting smoltcp init from main.rs), in a separate PR altogether?

@@ -0,0 +1,251 @@
// Ripped off from minimq example
Copy link
Member

@jordens jordens Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should really be its own crate or in enc424j600 (like it is in w5500) I think. And the code provenience/copyright needs to be clearly explained.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will add the NAL support to enc424j600.

Comment on lines 225 to 230
/// Override MAC address during initialization
pub fn set_mac(mut self, mac: [u8; 6]) -> Self {
// MAC address is designed to be unchanged in EEPROM
self.eui48.copy_from_slice(&mac);
self
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is broken I think. It'll guarantee that the checksum is always invalid.

Copy link
Member

@ryan-summers ryan-summers Jan 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MAC isn't actually saved to EEPROM. Only BoosterMainBoardData is. The MAC is always read form the EUI-48 section, so there's no need to duplicate that data into EEPROM writable region.

Edit: Post-discussion with Robert revealed that this statement is incorrect - the MAC is actually used in salting the CRC32 calculation.

use w5500::MacAddress;

#[cfg(feature = "phy_enc424j600")]
use super::mac::MacAddress;
Copy link
Member

@jordens jordens Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MacAddress should either be in the enc424j600 crate (like it is in the w5500 crate). Or it should be in smoltcp-nal, or in smoltcp. No need to copy and paste it here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a3db62f, using 'EthernetAddress' from smoltcp as 'MacAddress'.

src/main.rs Outdated

#[cfg(feature = "phy_enc424j600")]
{
SerialTerminal::new(usb_device, usb_serial, settings.set_mac(eth_mac_addr))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get the reason for the added code.
Setting the MAC doesn't have much to do with creating the serial terminal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to relay MAC address from the one in enc424j600 to the serial terminal, we don't use the one on the eeprom.
Just for the read mac command.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this doesn't factor into the checksum computation on settings save? Did you test that?

src/settings/mac.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated
Comment on lines 49 to 50
git = "https://github.com/smoltcp-rs/smoltcp.git"
rev = "2080152b"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be smoltcp 0.7.0 and the patch removed?

Cargo.toml Outdated

# Coerce ENC424J600 to use the latest smoltcp to accomodate minimq
[patch.crates-io]
smoltcp = { git = "https://github.com/smoltcp-rs/smoltcp.git", rev = "2080152b" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should capture the revision in the Cargo.lock file - is there a specific reason why this commit is important? If so, we need to add documentation around it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be bumped to 0.7.0 after the NAL is incorporated into the enc424j600 crate.

use enc424j600::EthController;
use smoltcp as net;

match enc424j600.init_dev(&mut delay) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplify to enc424j600.init_dev(&mut delay).expect("PHY initialization failed");

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e66d916.

let eth_iface = unsafe {
let device = enc424j600::smoltcp_phy::SmoltcpDevice::new(enc424j600);

// TODO: Restore IP config from EEPROM
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this TODO still relevant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in e66d916.

};

let routes = {
// TODO: Restore gateway config from EEPROM
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this TODO still relevant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in e66d916.

}

let mut eth_mac_addr: [u8; 6] = [0; 6];
enc424j600.read_from_mac(&mut eth_mac_addr).unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly not a fan of using different MACs for the different PHYs. The MAC is derived from a EUI-48 identifier IC on the booster board, so in the case of the ENC424J600, we technically have two MAC addresses that we can use. Does the ENC424J600 allow us to override the MAC address with our own? That would simplify the system-level logic to always use the EUI48 identifier, as that is also used in identifying booster.

Copy link
Member

@ryan-summers ryan-summers Jan 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff actually becomes a lot smaller and keeps the code flow simpler if the ENC424J600 is updated to utilize boosters internal MAC address (while ignoring the ENC424J600's address).

The datasheet says this is possible:
image

This allows us to keep the change minimal, which increases maintainability in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. The MAC address overriding will be supported in enc424j600 crate.
We will use the EUI-48 ID from the eeprom.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryan-summers We would need to review some PRs on the ENC424J600 driver repo and do some cleanup first, before we can finalise this PR here. Thanks!

Comment on lines 225 to 230
/// Override MAC address during initialization
pub fn set_mac(mut self, mac: [u8; 6]) -> Self {
// MAC address is designed to be unchanged in EEPROM
self.eui48.copy_from_slice(&mac);
self
}
Copy link
Member

@ryan-summers ryan-summers Jan 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MAC isn't actually saved to EEPROM. Only BoosterMainBoardData is. The MAC is always read form the EUI-48 section, so there's no need to duplicate that data into EEPROM writable region.

Edit: Post-discussion with Robert revealed that this statement is incorrect - the MAC is actually used in salting the CRC32 calculation.

#[cfg(feature = "phy_enc424j600")]
{
MacAddress::from_bytes(&self.eui48)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The brackets are unnecessary here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3869217.

@@ -36,6 +36,20 @@ features = ["stm32f407", "rt", "usb_fs"]
[dependencies.w5500]
git = "https://github.com/quartiq/w5500"
branch = "feature/tcp-nal"
optional = true

[dependencies.enc424j600]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the .github/workflow/ci.yml file to build both PHYs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 4ae6e82

@@ -63,6 +77,13 @@ path = "tca9548"

[features]
unstable = []
phy_enc424j600 = [ "enc424j600", "smoltcp" ]
phy_w5500 = [ "w5500" ]
default = [ "phy_w5500" ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like removing this line (i.e. not defining default features entirely) would avoid situations like running cargo build --features=phy_enc424j600 but forgetting to use --no-default-features. I would like to hear the opinion from others on this, thanks!

@HarryMakes
Copy link
Contributor

Just an update: Since @occheung is on leave, I've been handling the addition of ENC424J600. I have an active PR on the ENC424J600 repo, so after that has been merged I'll probably open a new PR here on the Booster firmware repo with updated function calls to the driver/NAL. That new PR will have my own commits on top of all commits @occheung has pushed here.

@HarryMakes
Copy link
Contributor

Replaced with #154. Closing.

@HarryMakes HarryMakes closed this Apr 12, 2021
@jordens jordens deleted the feature/enc424j600 branch October 18, 2022 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants