Skip to content

Conversation

@jukkar
Copy link
Member

@jukkar jukkar commented Mar 25, 2018

As we have E70 ethernet driver enabled, we need to enable the
generic ethernet L2 driver too so that the ethernet is actually
useful.

Signed-off-by: Jukka Rissanen [email protected]

@codecov-io
Copy link

codecov-io commented Mar 25, 2018

Codecov Report

Merging #6789 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6789      +/-   ##
==========================================
- Coverage   54.99%   54.99%   -0.01%     
==========================================
  Files         456      456              
  Lines       43842    43842              
  Branches     8225     8225              
==========================================
- Hits        24112    24111       -1     
- Misses      16386    16387       +1     
  Partials     3344     3344
Impacted Files Coverage Δ
kernel/posix/pthread.c 69.5% <0%> (-0.5%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87d6c50...22df3bc. Read the comment docs.

@jukkar
Copy link
Member Author

jukkar commented Mar 26, 2018

This needs more thinking, lot of tests are failing here and there after activating L2 ethernet by default for E70.

@jukkar jukkar added the DNM This PR should not be merged (Do Not Merge) label Mar 26, 2018
@jukkar jukkar force-pushed the sam_e70_xplained_eth branch from 82660f0 to 8bbf74e Compare March 29, 2018 09:41
@jukkar jukkar removed the DNM This PR should not be merged (Do Not Merge) label Mar 29, 2018
@jukkar
Copy link
Member Author

jukkar commented Mar 29, 2018

New version fixing sanity issues.

@jukkar jukkar requested a review from tbursztyka as a code owner March 29, 2018 11:37
@jukkar
Copy link
Member Author

jukkar commented Mar 29, 2018

Some of the sanity tests need bigger network buffer limits when compiling for E70 Xplained board.

@jukkar
Copy link
Member Author

jukkar commented Apr 3, 2018

@nashif are you ok with this PR?

Copy link
Member

Choose a reason for hiding this comment

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

dont like this, just settle on a default that works for everyone, this is just a sample, so please try to have 1 prj.conf.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would have done that if that would have been easy. The E70 ethernet driver checks that there is enough network buffers available and gives error if not. Unfortunately we have several prj.conf files in samples/tests that have less buffers configured.
This means that we either

  • Remove the checks in E70 eth driver. I would like to avoid this as these checks make sense for this driver

or

  • We increase the buf counts in various samples. This is not good either as then some of the smaller memory boards will start to fail

or

  • Blacklist E70 in some of the failing tests. This is certainly an option but I preferred to just increase the buf counts for these couple of tests so that we get to compile them for E70.

If you have some other idea, please suggest.

Copy link
Member

Choose a reason for hiding this comment

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

i would go with options 2:

We increase the buf counts in various samples. This is not good either as then some of the smaller memory boards will start to fail

Copy link
Member

Choose a reason for hiding this comment

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

ditto

jukkar added 2 commits April 4, 2018 10:40
As we have E70 ethernet driver enabled, we need to enable the
generic ethernet L2 driver too so that the ethernet is actually
useful.

Signed-off-by: Jukka Rissanen <[email protected]>
Some of the sanitycheck tests were having too small limit for
network buffers when compiling for sam_e70_xplained board.
Increase the buffer limits when testing this for this board.

Signed-off-by: Jukka Rissanen <[email protected]>
@jukkar jukkar force-pushed the sam_e70_xplained_eth branch from 3f0d233 to 22df3bc Compare April 4, 2018 10:03
@jukkar
Copy link
Member Author

jukkar commented Apr 4, 2018

Increased the default number of buffers if ethernet is enabled. This allowed us to remove various net_buf configs from prj.conf files.

@jukkar
Copy link
Member Author

jukkar commented Apr 5, 2018

@nashif does this look better now?

@nashif nashif merged commit a0df4f6 into zephyrproject-rtos:master Apr 5, 2018
@pfalcon
Copy link
Contributor

pfalcon commented May 5, 2018

I missed this change. And it does not do what it advertises - it doesn't "Fix sanitycheck for sam_e70_xplained board". Instead, it works around bugs in the IP stack in a typical Zephyr way - by throwing large amounts of memory, to minimize probability of bugs and deadlocks of which the IP stack is full (#5857, etc.).

Neither it masks it only for sam_e70_xplained, it does it for all boards with NET_L2_ETHERNET, which affects quite a number of boards, starting with qemu_x86 (which uses SLIP, which is NET_L2_ETHERNET).

Some of the sanitycheck tests were having too small limit for
network buffers when compiling for sam_e70_xplained board.

Small limit of network buffers or large, it should work. Inability to work with small number of buffers points to bugs in the stack.

@pfalcon
Copy link
Contributor

pfalcon commented May 6, 2018

At the same time, some long-standing issues were actually resolved/alleviated. I was able to run net/sockets/big_http_download sample (transferring megabytes of data from the Internet) with quite minimal buffer settings with qemu_x86, and then was able to use the same settings with frdm_k64f and Ethernet: pfalcon#3 (comment) .

So, I'd suggest to revert this patch (specifically a0df4f6 commit), document what's wrong when running what samples on sam_e70_xplained, compare the behavior with other Ethernet boards like frdm_k64f, and then see if it's sam_e70_xplained specific issue, Ethernet specific issue, or bug in the IP stack.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants