drivers/ethernet: Adapt stellaris driver to new L2 behaviour #11869
Conversation
L2 is the one who requests the packet to be sent, and not via net_if API anymore. Stellaris driver was merged right after this behaviour change and was thus lacking the proper modification. Signed-off-by: Tomasz Bursztyka <[email protected]>
|
cc: @bravegnu |
These kind of passive-aggressive comments do not really help anybody. If you need the info, just ask if / how this has been tested. |
I asked in #10547 (comment) and got no reply. I asked yesterday (about testing in general) during the monthly meeting. @tbursztyka left by that time (I saw him leaving just as I spoke the question, to be exact). And you said that you don't remember details of a particular case to test, and in general, you don't do "stress testing". I expected another guy to talk about testing in general and testing of his TCP implementation in particular, and he just didn't show up. Oh, I wanted to ask if somebody uses the testsuite developed by that guy, but as everyone was apparently too keen to close that (last in the year, and thus summary-of-the-year) meeting early, I just didn't. Passive-aggressive comments you say? Let my just shut up then. |
Hmm, this was submitted under 1 hour ago, how some other PR is related to this one. |
In pretty obvious way IMHO - there're too many questions about testing, and too few answers. Whereas in the ideal case, there wouldn't be even questions, but information on the testing. But surely, that can be just my skewed perception, sorry. |
You would have read the commit message, you would have known why this situation happened. It's not L2 rework PR that was the culprit, but really the Stellaris ethernet driver's PR that was not rebased/tested once mine got in. |
|
Actually, I noticed the original PR introducing this driver also add the ethernet stats. which is good, but it's doing it in the wrong place, I missed that. Up to L2 to update such stats and not each and every driver |
Well, but the talk is not about this, but whether after this change, the stellaris ethernet driver was test to work (again). If that was done, that's high mark, and should be talked about, to encourage your other colleagues do the same with their changes (which is hard, we don't have enough hardware, time, etc.). And if it's not, it would be fair to ask @bravegnu (the author of the driver) to test it, and give him some time for that. So, my comments are very intent-based, and are not made to just stir up conflict. |
More routines for more ethernet stat events specifically (or what I see there). I actually waited for them to be merged for my #11680 too. So, if there's something wrong with that part (maybe you mean something else), I'd be interested to be in loop. |
What's wrong with it is the place where the stats are updated. It should be in subsys/net/l2/ethernet.c:ethernet_send(). I'll send a PR to fix this |
Thanks! And to say which might have fallen thru the cracks - thanks @tbursztyka for coming forward with updating this driver, and do it so promptly, instead of letting it start to bitrot - it's absolutely great! |
|
Ran a basic ping test, on this change. My initial observation is that for ping packets the "Bytes sent" does not get incremented. While the count increments in the original version of the driver. But the "Bytes sent" gets incremented for TCP packets, in both the versions. |
https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/ethernet/eth_native_posix.c is also updating the stats. Thought it was a missing feature in other drivers. |
|
@bravegnu: Thanks for testing! Great to know that the packet passing works as expected. Stats hopefully will be revamped by @tbursztyka as he said.
I noticed the same, and figured my stance on that would be that it's useful to have Eth-level stats for testing drivers, like native_posix, stellaris, and I decided to also maintain them in my wip smsc9118 driver. Anyway, if I understood @tbursztyka right, he plans to make Eth stats to work for any Eth driver automagically. |
Codecov Report
@@ Coverage Diff @@
## master #11869 +/- ##
==========================================
- Coverage 51.43% 51.3% -0.14%
==========================================
Files 231 231
Lines 28751 28752 +1
Branches 7152 7156 +4
==========================================
- Hits 14789 14751 -38
- Misses 11144 11188 +44
+ Partials 2818 2813 -5
Continue to review full report at Codecov.
|
|
@bravegnu the original is not going to compile on master branch so I guess you mean the original in your former dev branch. I don't think that patch affects anything about stats (note that I use data_len instead of net_pkt_get_len(), but data_len isn't modified as far as I see) |
|
@tbursztyka I agree that the issue was not introduced by this change. The stats issues needs to be fixed separately. |
L2 is the one who requests the packet to be sent, and not via net_if API
anymore. Stellaris driver was merged right after this behaviour change
and was thus lacking the proper modification.
Signed-off-by: Tomasz Bursztyka [email protected]