Skip to content

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Oct 12, 2016

This was taken out of #5802 and only provides the sock_udp part and the tests for it.

This contains duplicate changes to #5936, so merging any of these might require rebase.

@miri64 miri64 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 12, 2016
@miri64 miri64 mentioned this pull request Oct 12, 2016
5 tasks
@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 13, 2016
@miri64
Copy link
Member Author

miri64 commented Oct 31, 2016

Needs adaptation to #5945.

@miri64 miri64 added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 5, 2016
@kaspar030
Copy link
Contributor

No time, reassigning.

@kaspar030 kaspar030 assigned kYc0o and unassigned kaspar030 Nov 18, 2016
@miri64
Copy link
Member Author

miri64 commented Nov 18, 2016

Adaption to #6137 (comment) also needed...

@kYc0o
Copy link
Contributor

kYc0o commented Nov 29, 2016

What's the latest on this?

@miri64 miri64 force-pushed the pkg/feat/lwip-sock-udp branch from 5a1e6b2 to 05f6a12 Compare November 29, 2016 19:28
@miri64
Copy link
Member Author

miri64 commented Nov 29, 2016

Rebased and made adaptation.

@miri64 miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 29, 2016
@miri64
Copy link
Member Author

miri64 commented Jan 13, 2017

Ping?

@miri64 miri64 force-pushed the pkg/feat/lwip-sock-udp branch 2 times, most recently from 5125e2b to 0be4ebb Compare January 17, 2017 12:39
@miri64
Copy link
Member Author

miri64 commented Jan 17, 2017

Rebased and squashed to latest master.

@miri64 miri64 force-pushed the pkg/feat/lwip-sock-udp branch 2 times, most recently from ef282dc to 0be4ebb Compare January 17, 2017 12:45
@miri64 miri64 force-pushed the pkg/feat/lwip-sock-udp branch from a2c8f03 to 1f83319 Compare January 24, 2017 14:33
@miri64
Copy link
Member Author

miri64 commented Jan 24, 2017

Squashed

@miri64 miri64 force-pushed the pkg/feat/lwip-sock-udp branch from 1f83319 to e801839 Compare January 24, 2017 15:34
@miri64
Copy link
Member Author

miri64 commented Jan 24, 2017

Added nucleo32-f042 to boards that do not have sufficient memory and squashed immediately.

@miri64
Copy link
Member Author

miri64 commented Jan 24, 2017

Jenkin's happy. I'm adding Murdock :-)

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 24, 2017
@PeterKietzmann
Copy link
Member

I guess it's known that tests/lwip and examples/gnrc_networking is not interoperable?

@PeterKietzmann
Copy link
Member

On a phytec board I don't get an IPv6 address. ps() crashes the node but this is probably an issue of #6372.

@kYc0o
Copy link
Contributor

kYc0o commented Jan 26, 2017

Well I think for interoperability lwip is using IPv4 and gnrc_networking IPv6 isn't it? @miri64 ?

@miri64
Copy link
Member Author

miri64 commented Jan 26, 2017

I guess it's known that tests/lwip and examples/gnrc_networking is not interoperable?

Yes, there is a bug in lwIP preventing this

Well I think for interoperability lwip is using IPv4 and gnrc_networking IPv6 isn't it? @miri64 ?

Huh? lwIP also is able to speak IPv6, but its not parsing IEEE 802.15.4 headers correctly.

@miri64
Copy link
Member Author

miri64 commented Jan 26, 2017

@PeterKietzmann are the extensive tests in tests/lwip_sock_udp working?

@PeterKietzmann
Copy link
Member

As said, tests/lwip_sock_udp and with #6372 and tests/lwip I was able to send local udp packets between two atmel nodes. ps() crashes nodes (more related to #6372. I guess) and data transmission between GNRC and LWIP does not work (known issue). I just scanned over the code which looked ok at the first sight. I hoped to get @kYc0o's opinion about this PR and merging. BTW: I will announce feature freeze soooon

Copy link
Contributor

@kYc0o kYc0o left a comment

Choose a reason for hiding this comment

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

Just some minor comments, I'll test and then ACK.


ssize_t sock_udp_recv(sock_udp_t *sock, void *data, size_t max_len,
uint32_t timeout, sock_udp_ep_t *remote)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ssize_t? As far as I can see you're returning int and at the end you cast another int to this type...

Copy link
Member Author

Choose a reason for hiding this comment

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

This is unrelated to this PR. If you want to change the sock API, please do so in a separate PR. Otherwise I can't change it, because the API demands it.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I didn't know that the sock API forced that type. Then for this PR it's OK.

Copy link
Member

Choose a reason for hiding this comment

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

Please don't change the API again :-(

#define _MSG_QUEUE_SIZE (1)
#define _SEND_DONE (0x92d7)
#define _NETDEV_BUFFER_SIZE (128)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe these could be aligned?

@kYc0o
Copy link
Contributor

kYc0o commented Feb 7, 2017

Tested with success on native.

@miri64
Copy link
Member Author

miri64 commented Feb 7, 2017

Adressed comments

@kYc0o
Copy link
Contributor

kYc0o commented Feb 7, 2017

Then everything is OK, you squash and merge when Murdock agrees.

Copy link
Contributor

@kYc0o kYc0o left a comment

Choose a reason for hiding this comment

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

ACK

@PeterKietzmann
Copy link
Member

@kYc0o did you experience the same problems with ps() on native as I described above?

@kYc0o
Copy link
Contributor

kYc0o commented Feb 7, 2017

I'll check it asap, since I just ran make test and all the tests passed.

@miri64 miri64 force-pushed the pkg/feat/lwip-sock-udp branch from 83407ea to a45256a Compare February 7, 2017 14:48
@miri64
Copy link
Member Author

miri64 commented Feb 7, 2017

Squashed

@kYc0o did you experience the same problems with ps() on native as I described above?

@PeterKietzmann I highly doubt this is related to this PR.

@kYc0o
Copy link
Contributor

kYc0o commented Feb 7, 2017

@PeterKietzmann I don't find a way to test ps() since the only example provided is the test, which just call and test the functions, I don't have access to any shell... You want me to modify the test to call ps() at the end?

@miri64
Copy link
Member Author

miri64 commented Feb 7, 2017

@kYc0o Peter was refering to the test provided in #6372.

@PeterKietzmann
Copy link
Member

As said, tests/lwip_sock_udp and with #6372 and tests/lwip I was able to send local udp packets between two atmel nodes. ps() crashes nodes (more related to #6372. I guess) and data transmission between GNRC and LWIP does not work (known issue).

@miri64, @kYc0o I've tested with #6372 and I would also assume the problem in there. However, I intended to compare test results. We can continue the discussion in there.

@miri64
Copy link
Member Author

miri64 commented Feb 7, 2017

Yapp, but are you okay with merging this one for now?

@kYc0o
Copy link
Contributor

kYc0o commented Feb 7, 2017

I'd say merge everything necessary for #6732 and test there, if there is something pointing here then we can find it easily I think.

So, merge at will.

@PeterKietzmann
Copy link
Member

@kYc0o did I scare you to hit the button :-) ? I wanted to motivate more detailed testing. However, I'm fine with merging this one now but please provide help in #6732.

@PeterKietzmann PeterKietzmann merged commit e5e79c2 into RIOT-OS:master Feb 8, 2017
@miri64 miri64 deleted the pkg/feat/lwip-sock-udp branch February 8, 2017 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants