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

netdev2_test: initial import #4835

Merged
merged 2 commits into from
Mar 30, 2016

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Feb 16, 2016

Imports a generic framework to test and experiment with netdev2-based modules.

@miri64 miri64 added Area: network Area: Networking Area: tests Area: tests and testing framework Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Feb 16, 2016
@miri64 miri64 added this to the Release 2016.03 milestone Feb 16, 2016
@miri64
Copy link
Member Author

miri64 commented Feb 16, 2016

@PeterKietzmann maybe of interest to you for the next iteration of netdev2_reflector.

@miri64
Copy link
Member Author

miri64 commented Feb 16, 2016

@thomaseichinger @OlegHahm @rousselk and others might also be interested for testing their MAC implementations.

@PeterKietzmann
Copy link
Member

Yes I am, thanks! Might take some days to review. But somehow the name (netdev2_test) and maybe also the location of that file seem strange to me.

@miri64
Copy link
Member Author

miri64 commented Feb 17, 2016

Well it's a test framework and a network library. Where else should it go and be called in your opinion?

@PeterKietzmann
Copy link
Member

Well, I know it's not really concrete but ideas going through my head:

  • How about naming it something like netdev2_stub, netdev2_nolink, netdev2_empty
  • If I would have assumend it in the drivers folder, similar to other network devices implementing netdev2
  • I'm wondering if it might be helpful to add a small test application, even if it won't do anything :-). Just as a showcase

What do you think?

@miri64
Copy link
Member Author

miri64 commented Feb 18, 2016

How about naming it something like netdev2_stub, netdev2_nolink, netdev2_empty

IMHO it's neither a stub, nor lacking a link, nor is it empty. Because of the callbacks it's quite powerful and could even be used to write (or [partially] wrap) a real driver.

If I would have assumend it in the drivers folder, similar to other network devices implementing netdev2

But it is no device, but a library. Though it's position is special netdev2_tap isn't in drivers either.

I'm wondering if it might be helpful to add a small test application, even if it won't do anything :-). Just as a showcase

I gave a small example in the doc, but sure a test application for a full example might be helpful.

* }
*
* int main(void) {
* ipv6_addr_t dst = IPV6_ADRD_UNSPECIFIED;
Copy link
Contributor

Choose a reason for hiding this comment

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

ADDR?

@kaspar030
Copy link
Contributor

I'm not sure I quite get the concept.
Is this a layer that can be used to override any of the driver fields and any of the get/set handlers?

@miri64
Copy link
Member Author

miri64 commented Feb 18, 2016

I'm not sure I quite get the concept.
Is this a layer that can be used to override any of the driver fields and any of the get/set handlers?

Don't think of it as a layer but a test dummy. It allows for total control of what should happen at the device level while providing an interface that is exactly the same to a normal netdev2. I had this concept going through my mind for quite a while now in regards to automated testing, but I realized that it also can be quite nifty for the experimentations I need to do for my thesis (mainly time messurements but maybe also energy consumption).

@kaspar030
Copy link
Contributor

It allows for total control of what should happen at the device level while providing an interface that is exactly the same to a normal netdev2.

Whats the advantage over just defining a netdev2_t whose "driver" passes through stuff to a (possible) real driver?

@miri64
Copy link
Member Author

miri64 commented Feb 19, 2016

It's

a) platform independent (no need for a dependency on a specific netdev)
b) hot-pluggable (scenarios can be switched between tests)

@miri64
Copy link
Member Author

miri64 commented Feb 19, 2016

And again: don't think of it as an overriding or adaption layer between a stack and a device, but a separate device. Just because you can override functionality of an existing device with this does not mean you have to. My showcase will hopefully demonstrate this.

@kaspar030
Copy link
Contributor

a) platform independent (no need for a dependency on a specific netdev)

netdev2_t netdev_test {
   netdev2_t driver;
   { netdev_test fields }
   netdev2_t *possible_sub_dev;
}

done.

b) hot-pluggable (scenarios can be switched between tests)
If you re-use the getter/setter array from this PR, you can have the same granularity of dynamically adding get/set methods. But do you intend of doing that? Aren't you probably exchanging whole sets?
No need for getters/setters for _send(), recv(), ..., just (possibly locking once) write to the structs driver member, which dosn't have to be static.

I think this can be simplified a lot.

@miri64
Copy link
Member Author

miri64 commented Feb 19, 2016

netdev2_t netdev_test {
    netdev2_t driver;
    { netdev_test fields }
    netdev2_t *possible_sub_dev;
}

done.

Since I don't get what this has to do with this module... what it netdev2_test fields? How are they composed? How do I reperesent a device that has an l2addr length of 8 AND a device that has no l2addr at all with this to simulate a border router e.g.? What is possible_sub_dev for? Again: This is not a bridge or overriding layer! This is primarily a test framework.

@miri64
Copy link
Member Author

miri64 commented Feb 19, 2016

Or do you mean that netdev2_test fields should be an array of NETOPT_NUMOF values of an arbitrary length? Isn't this a bit RAM bloating? What about checking send/recv behavior? I already done this with netdev (the predecessor of gnrc_netdev) and it was a nightmare to handle mutexes on this fields in the test application.

@miri64
Copy link
Member Author

miri64 commented Feb 19, 2016

Also would time measurements be done in this scenario?

@miri64
Copy link
Member Author

miri64 commented Feb 19, 2016

Regardless of this discussion: I will use this framework for my experiments. I only have 5 weeks left for my thesis so not much time to redo stuff...

@kaspar030
Copy link
Contributor

This is not a bridge or overriding layer! This is primarily a test framework.

It's netdev2 with getters/setters for the driver struct and an array for get/set handlers. Maybe that is handy.

@miri64
Copy link
Member Author

miri64 commented Feb 21, 2016

Added a test application.

@PeterKietzmann
Copy link
Member

I'm not available during the next days. @kaspar030 would you take this PR over? You are already involved in discussions and Martine is in a hurry...

@miri64
Copy link
Member Author

miri64 commented Feb 24, 2016

Rebased and adapted to current master.

@miri64
Copy link
Member Author

miri64 commented Mar 29, 2016

Done.

@miri64
Copy link
Member Author

miri64 commented Mar 29, 2016

or Fixed.

@PeterKietzmann
Copy link
Member

Confirmed that the tests succeeds now. I need to further look into your code.

mutex_lock(&dev->mutex);
if (dev->recv_cb != NULL) {
/* could fire context change and call _recv so we need to unlock */
mutex_unlock(&dev->mutex);
Copy link
Member

Choose a reason for hiding this comment

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

I see why you unlock so mutex before recv_cb but what if recv_cb == NULL? Don't you need to unlock the mutex in that case as well?

@PeterKietzmann
Copy link
Member

I would have liked some more control messages in the application to better understand the program flow. E.g. one before a netdev action happens and one message per callback to see some kind of action-response scheme. But I leave it up to you how to improve that :-)

void *info);

/**
* @brief Callback type to handle device initialization.
Copy link
Member

Choose a reason for hiding this comment

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

Well, it's picky but sometimes you end the line with a dot and sometimes you don't

@PeterKietzmann
Copy link
Member

@authmillenon we're not far from ACKing :-)!

@miri64
Copy link
Member Author

miri64 commented Mar 30, 2016

I would have liked some more control messages in the application to better understand the program flow. E.g. one before a netdev action happens and one message per callback to see some kind of action-response scheme. But I leave it up to you how to improve that :-)

What do you mean by control messages? Isn't messaging a little bit to complicated for this simple test application?

@miri64
Copy link
Member Author

miri64 commented Mar 30, 2016

Addressed comments

@OlegHahm OlegHahm added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable and removed Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties labels Mar 30, 2016
@PeterKietzmann
Copy link
Member

I didn't mean messages in terms of IPC but as "debug messages" to STDOUT. IMO it is not too simple. For example: I also put some printfs here and there and stepped through the application code to see what is happening before the output says "test successful". Besides of that: please squash

Imports a generic framework to test and experiment with netdev2-based
modules.
@miri64
Copy link
Member Author

miri64 commented Mar 30, 2016

Done.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Mar 30, 2016
@PeterKietzmann
Copy link
Member

/data/riotbuild/drivers/include/periph/timer.h:24:24: fatal error: periph_cpu.h: No such file or directory
for qemu-i386. Any ideas how to solve? Blacklist?

@PeterKietzmann PeterKietzmann 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 Mar 30, 2016
@miri64
Copy link
Member Author

miri64 commented Mar 30, 2016

Black list I would say. Will fix.

@miri64
Copy link
Member Author

miri64 commented Mar 30, 2016

Added periph_timer as a required feature. But alternatively we could just provide an (empty) periph_cpu.h to qemu in a separate PR.

@PeterKietzmann
Copy link
Member

Good idea! Then -finally- let's get this in. ACK and go

@PeterKietzmann PeterKietzmann merged commit dcafbb8 into RIOT-OS:master Mar 30, 2016
@miri64 miri64 deleted the netdev2_test/feat/init branch March 30, 2016 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR 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.

None yet

4 participants