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 IPv4 support for RIOT OS #129

Merged
merged 1 commit into from
Jun 24, 2022
Merged

Conversation

HendrikVE
Copy link
Contributor

@HendrikVE HendrikVE commented Mar 26, 2022

Add the capability to communicate using IPv4.

PR in RIOT OS has already been approved and was merged:

Dependencies:

I'll rebase once the dependencies are merged.

Off-topic:
At first glance the project seemed to be stale, but after looking into the develop branch it became clear it is not. I just wanted to point out that some first-time users (like me) might get the wrong impression when they see that the latest commit to master is from July 2020 ;)

@boaks
Copy link
Contributor

boaks commented Mar 28, 2022

In order to contribute to a Eclipse project, you need to sign a ECA, see CONTRIBUTING.

Edit: sorry, I was wrong, it's just the PR you depend on, which needs the ECA.

@boaks
Copy link
Contributor

boaks commented Mar 29, 2022

Is that common in RIOT, to select IPv4 or IPv6 at compile time?
If not, I would consider a more generic solution, which decides that at runtime.
That would also make it easier, to use tinydtls as link-library.

@HendrikVE
Copy link
Contributor Author

Is that common in RIOT, to select IPv4 or IPv6 at compile time?

Yes, RIOTs sock abstraction for instance will provide the IPv6 member conditionally at compile time, with all of its implications. At the moment the IPv4 member will be always present, so checking for SOCK_HAS_IPV4, which was just introduced recently, is not strictly necessary, but that may change later.
In the end it is not an issue of availability of members in tinydtls: ipv6_addr_t and ipv6_addr_equal for example will be always available regardless the compile time options. However without compile time options, in the case of IPv4-only that would add 208 bytes to the .text section and for IPv6-only it would add 44 bytes without the need for it and I experienced a few times that the RIOT community is quite pedantic when it comes to memory usage.

@boaks
Copy link
Contributor

boaks commented Mar 29, 2022

I'm not sure, if I get it. It sounds, that IPv6 must be enabled at compile time, but the other sentence seems to indicate, that IPv4 is always usable.

So, the configurations are:

  • IPv4 (only)
  • IPv4 and IPv6 (if enabled at compile time)

Or am I mislead?

@HendrikVE
Copy link
Contributor Author

HendrikVE commented Mar 29, 2022

I had a closer look on the codebase. As far as I can tell all important parts that would require IPv4 are left out conditionally by the compile-time switch SOCK_HAS_IPV4. The lines of codes that are not explicitly protected by it are covered by checking the family type for AF_INET. The confusing part is the fact that the IPv4 address member is within a union with IPv6, thus making no difference at this point, but its existence in a IPv6-only mode does not make sense and should also be removed. But these are things to be improved in RIOTs codebase.

So in summary we have all possible configurations:

  • IPv4-only
  • IPv6-only
  • IPv4 and IPv6 dualstack

@obgm
Copy link
Contributor

obgm commented May 23, 2022

@HendrikVE can you please rebase?
Sorry, maybe we just wait for PR #127.

@obgm
Copy link
Contributor

obgm commented May 23, 2022

Looks good, ready to merge right after doing PR #127.

@HendrikVE HendrikVE force-pushed the pr/add_ipv4 branch 2 times, most recently from b2ca395 to 4cb2832 Compare May 25, 2022 23:29
@HendrikVE
Copy link
Contributor Author

Rebased :)

(Sorry for force-pushing multiple times, I tried to use another email address of mine, but ECA failed with it)

session.h Show resolved Hide resolved
@boaks
Copy link
Contributor

boaks commented Jun 24, 2022

LGTM

@boaks boaks merged commit dd31ec9 into eclipse:develop Jun 24, 2022
@boaks
Copy link
Contributor

boaks commented Jun 24, 2022

merged to develop, cherry picked to main.

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.

3 participants