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

use core::net types for IpAddress #1000

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aurelj
Copy link

@aurelj aurelj commented Oct 16, 2024

Similar to what was done for Ipv4Address and Ipv6Address.

  • use core::net::IpAddr as the ip address type
  • remote v4() and v6() constructor, use core IpAddr::V4(Ipv4Addr::new(_)) instead
  • Remove IpVersion from public API

@Dirbaio
Copy link
Member

Dirbaio commented Oct 16, 2024

I intentionally did not do this because it will cause extra RAM usage when you enable IPv4 only. We conditionally enable the enum cases so IpAddress is only 4 bytes if you enable IPv4 only, while core::net::IpAddr will be 16 bytes + the enum tag.

@aurelj aurelj force-pushed the core_net_ipaddr branch 2 times, most recently from e6623e0 to 7554dea Compare October 16, 2024 12:45
@aurelj
Copy link
Author

aurelj commented Oct 16, 2024

OK, I see, this is a fair point.

My vision is that embedded systems that use an IP stack are generally not the most memory constrained one, and that they usually won't handle more that one or two IpAddress, so I hardly see this as a practical issue. Also, I guess more and more systems are enabling IPv6 and thus won't save the 12 bytes or RAM anyway.

I'm not sure what is the policy in smoltcp, but if it is to try to squeeze up to the last bit than can be squeezed, then I agree this PR is a bad idea.

@aurelj aurelj force-pushed the core_net_ipaddr branch 3 times, most recently from baba4ea to 908d774 Compare October 16, 2024 15:53
@Dirbaio
Copy link
Member

Dirbaio commented Oct 16, 2024

there's also the question of what happens when you pass an IP addr to the stack that is of a version that is not enabled. Before this was imposssible, now it'll need to either return an error or panic.

@aurelj aurelj force-pushed the core_net_ipaddr branch 2 times, most recently from aa2c961 to 0690d43 Compare October 16, 2024 16:11
Similar to what was done for Ipv4Address and Ipv6Address.
- use core::net::IpAddr as the ip address type
- remote v4() and v6() constructor, use core IpAddr::V4(Ipv4Addr::new(_)) instead
- Remove IpVersion from public API
@crawford
Copy link
Contributor

My vision is that embedded systems that use an IP stack are generally not the most memory constrained one

My system has 4kiB of RAM, so I’d like just about every byte I can find.

@aurelj
Copy link
Author

aurelj commented Oct 17, 2024

My vision is that embedded systems that use an IP stack are generally not the most memory constrained one

My system has 4kiB of RAM, so I’d like just about every byte I can find.

Wow, a 4kiB MCU with an IP stack ! That's seriously impressive ! Curious what kind of MCU it is ? And does it have an internal ethernet MAC device or do you use an external ethernet MAC ?

Anyway, I agree that this PR is not desirable for such use case.

@crawford
Copy link
Contributor

Hang on. I’m an idiot. I’m mixing up my projects. This latest one uses the EFM32GG11, which has 512 KiB. On top of that, I see I only allocate space for a single address, so that 12-byte overhead doesn’t seem that bad (for my configuration).

I retract my statement. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants