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

ports/rp2/boards: add new definition files for W5500_EVB_PICO board. #8962

Closed
wants to merge 5 commits into from
Closed

ports/rp2/boards: add new definition files for W5500_EVB_PICO board. #8962

wants to merge 5 commits into from

Conversation

omogenot
Copy link
Contributor

@omogenot omogenot commented Jul 26, 2022

rp2/boards/W5500_EVB_PICO: directory added.
rp2/boards/W5500_EVB_PICO/board.json: file added.
rp2/boards/W5500_EVB_PICO/mpconfigboard.cmake: file added.
rp2/boards/W5500_EVB_PICO/mpconfigbaoard.h: file added.
rp2/boards/W5500_EVB_PICO/readme.md: file added.

Signed-off-by: github@mymeterinfo.info

rp2/boards/W5500_EVB_PICO: directory added
rp2/boards/W5500_EVB_PICO/board.json: file added
rp2/boards/W5500_EVB_PICO/mpconfigboard.cmake: file added
rp2/boards/W5500_EVB_PICO/mpconfigbaoard.h: file added
rp2/boards/W5500_EVB_PICO/readme.md: file added

Signed-off-by: github@mymeterinfo.info
@omogenot omogenot changed the title ports/rp2/boards: add new definition files for W5500_EVB_PICO board ports/rp2/boards: add new definition files for W5500_EVB_PICO board. Jul 26, 2022
@robert-hh
Copy link
Contributor

The commit message (not the PR message) should e.g. be formatted as:

ports/rp2/boards: Add new definition files for W5500_EVB_PICO board.

Capital A at Add and a full stop at the end. You can change that locally with:

git commit --amend

And then push again with:

git push -f origin W5500_EVB_PICO

Assuming that in your set-up the remote repository is called origin.

@omogenot
Copy link
Contributor Author

The commit message (not the PR message) should e.g. be formatted as:

ports/rp2/boards: Add new definition files for W5500_EVB_PICO board.

Capital A at Add and a full stop at the end. You can change that locally with:

git commit --amend

And then push again with:

git push -f origin W5500_EVB_PICO

Assuming that in your set-up the remote repository is called origin.

Thanks for your help. It's really driving me nuts. However, I did the git commit -amend, but the git push return a Everything up-to-date message and nothing is updated in Github.

@robert-hh
Copy link
Contributor

git push -f

-f like force update.

@omogenot
Copy link
Contributor Author

Yes I did. In fact I copied/pasted your command:

omogenot@OMMBP micropython % git push -f origin W5500_EVB_PICO
Everything up-to-date

@robert-hh
Copy link
Contributor

Does
git log
show the changed commit?
And what is the output of
git remote -v

@omogenot
Copy link
Contributor Author

No. git log shows only the previous (faulty) one.

omogenot@OMMBP micropython % git remote -v
origin	https://github.com/omogenot/micropython.git (fetch)
origin	https://github.com/omogenot/micropython.git (push)

rp2/boards/W5500_EVB_PICO: Directory added.
rp2/boards/W5500_EVB_PICO/board.json: File added.
rp2/boards/W5500_EVB_PICO/mpconfigboard.cmake: File added.
rp2/boards/W5500_EVB_PICO/mpconfigbaoard.h: File added.
rp2/boards/W5500_EVB_PICO/readme.md: File added.

Signed-off-by: github@mymeterinfo.info
@robert-hh
Copy link
Contributor

When you run
git commit --amend
you should get a editor with the old commit message, which has to be changed and saved.

@robert-hh
Copy link
Contributor

robert-hh commented Jul 26, 2022

You have three commit now. The second one was fine, but the last one with a merge is wrong again.

rp2/boards: Merge branch of https://github.com/omogenot/micropython into W5500_EVB_PICO.

Signed-off-by: github@mymeterinfo.info
@omogenot
Copy link
Contributor Author

I don't know why again ! But anyway. All this time spent (you and me) for a missing .

I have a simple fix to apply to allow IRQ usage with WIZNET5K object. in file extmod/network_wiznet5k.c the line 690 shall be removed (i.e. mp_arg_check_num(n_args, n_kw, 0, 3, false);) because this check is done later in the code at line 718 and 720.
Can you do me a favour and be so kind to do it for me. I can't spend another 2 hours just for this simple fix.
I thank you in advance.

I will abandon this PR (again), and may be create a new one for the mDNS support I am currently writing. But this PR procedure is way too complicated. I would have loved to contribute...

@robert-hh
Copy link
Contributor

git is sometimes really challenging. Just leave the PR open. A failed commit message check is not the worst that can happen.

@robert-hh
Copy link
Contributor

I have a simple fix to apply to allow IRQ usage with WIZNET5K object. in file extmod/network_wiznet5k.c the line 690 shall be removed (i.e. mp_arg_check_num(n_args, n_kw, 0, 3, false);) because this check is done later in the code at line 718 and 720.
Can you do me a favour and be so kind to do it for me.

Technically I can do that, But I would have to test it before. I used WIZNET5500 only with a PyBoard until now. But the change should affect that combination as well.

@omogenot
Copy link
Contributor Author

Well, just spend 2 minutes to have a look at these lines in the code, and you will easily see that this line was left there by mistake and blocks the ability of adding an optional Irq pin parameter if using LWIP (which handles this irq).
With this IRQ, a ping has a delay of less than 1 ms, as opposed to 40 to 65 ms without.
Anyway, I won't force you to do it. It was just to help.

@robert-hh
Copy link
Contributor

So I looked at the code, but cannot use it with PyBoard, which uses a different function for make new. I could use a genuine rp2040 and connect the lwip board to it. That may mimic a W5500_EVB_PICO board.

@robert-hh
Copy link
Contributor

So this combo works. Checking against your PR, there is no dhcp() method.

>>> dir(nic)
['__class__', 'active', 'config', 'ifconfig', 'isconnected', 'regs', 'send_ethernet', 'status']

nic.ifconfig() returns ('0.0.0.0', '0.0.0.0', '0.0.0.0', '0.0.0.0') after instantiation, but after nic.active(1), dhcp gets active and fetches an IP address.
Ping to the board has a trip time of ~0.75 ms with the default pins.

@omogenot
Copy link
Contributor Author

Hum, funny. I did not notice it because this comes from the W5100S_EVB_PICO definition files. It probably does not make sense there either.
I will have a look at the source to see where it comes from.

@robert-hh
Copy link
Contributor

Manual setting of the interface parameters works. Test script:

import network
from machine import Pin, SPI
nic=network.WIZNET5K()
spi=SPI(0)
nic=network.WIZNET5K(spi, Pin(17, Pin.OUT, value=1), Pin(20, Pin.OUT, value=1), Pin(21, Pin.IN))
nic.active(1)

Ping times without INTN ~34ms, with INTN ~0.8 ms.

@omogenot
Copy link
Contributor Author

Way cool !
Actually, according to the code, nic.ifconfig("dhcp") shall start a new DHCP discovery.

@robert-hh
Copy link
Contributor

Two things seen while testing:

  • my little uping.py to ping from in to out fails. No ping returned.
  • my little speed test script stops after a while. Setup: Small script on the board(below) which send to a (Linux) PC, which simple has the command nc -l 1234|wc -c running.
    Script:
import socket
from time import sleep_us, ticks_ms
blen = 1024
b = bytearray(blen)

def xmit(count, delay=1000):
    s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    s.connect(("10.0.0.152", 1234))
    mod = count // 100
    if mod < 10:
        mod = 10
    if mod > 10000:
        mod = 10000
    start = ticks_ms()
    sent = 0
    for i in range(count):
        try:
            s.sendall(b)
            sent += blen
            sleep_us(delay)
            if (i % mod) == 0:
                print(i)
        except Exception as e:
            print(i)
            print(e)
            break
    stop = ticks_ms()
    s.close()
    print(sent, "bytes sent", (stop - start)/count, "ms/frame")

10.0.0.152 it the IP of the PC.
I'll check for the ping thing tomorrow. My little udp test works.

@omogenot
Copy link
Contributor Author

In the meantime, I discovered a case during the init that could brick the pico device:

from network import WIZNET5K
nic = WIZNET5K()
nic.active(True)

The system hangs and is bricked.
To prevent it, we have to add a wiznet5k_deinit(); statement in the wiznet5k_init function before calling wiznet5k_lwip_init
Of course, as nothing is simple in this world, we have to do a forward declaration of wiznet5k_deinit before wiznet5k_init

@robert-hh
Copy link
Contributor

I had seen bricks a few time when configured with the Pin arguments without the Pin mode being set.
A little ftp server works, after small changes. It turns out that network.route() returns an empty list. It should return a nic list, which it does e,g, PyBoard and mimxrt).
And after nic.active(1) it takes a while until the IP is assigned.

@robert-hh
Copy link
Contributor

The network.route() thing is found & fixed. The call for registering the nic was missing in the LWIP path.

@omogenot
Copy link
Contributor Author

@robert-hh : Good job, is it related to the bug I found, that bricks the device when .active(True) is called before doing a .active(False) just after creating a new WIZNET5K object? Does it mean that my "fix" consisting in doing a call to wiznet5k_deinit inside the wiznet5k_init function before the call to wiznet5k_lwip_init? Or is it still relevant?

@robert-hh
Copy link
Contributor

robert-hh commented Jul 27, 2022

As for Ping: Instead of a ICMP message, a UDP datagram containing the ICMP message is sent. So it seems that SOCK_RAW is not supported, or supported in a different way.
Edit:
The test script opened the socket in DGRAM mode. Opening it in RAW mode and adding the proper checksum cured that. Ping time ~3 ms.

@robert-hh
Copy link
Contributor

is it related to the bug I found, that bricks the device when .active(True) is called before doing a .active(False) just after creating a new WIZNET5K object? Does it mean that my "fix" consisting in doing a call to wiznet5k_deinit inside the wiznet5k_init function before the call to wiznet5k_lwip_init

It may be related. I cannot tell at first glance. It is a pretty lengthy chain of calls which are made. My fix just adds:

    // register with network module
    mod_network_register_nic(&wiznet5k_obj);

at the end of the function wiznet5k_init(). But you could move that as a trial to the begin of the function. My guess is that the lock-up happens because the IRQ fires before completely set-up.

@omogenot
Copy link
Contributor Author

OK. However, my tests show that the lock-up occurs in wiznet5k_active when calling reg_wizchip_cris_cbfunc (which seem irrelevant if you look at the corresponding code, may be due to the timing or to delay in performing printf for tracing calls), so before calling wiznet5k_init...
I will try my tests again. I propose the following scenario:
1/ remove my modification: back to original bug
2/ remove IRQ pin in object creation
3/ add your modification
4/ add IRQ pin
And I will let you know.

@omogenot
Copy link
Contributor Author

OK. Here are the results...
First of all, it has nothing to do with the IRQ pin usage or not. That's good news. Please note that the default WIZNET5K object creation without parameters is enabling IRQ pin.
To reproduce the brick bug, do the following:

from network import WIZNET5K
nic = WIZNET5K()  # Use default SPI and Pins assignment
nic.active(True)
del nic
nic = WIZNET5K()
nic.active(True)

After the second nic.active(True), the device hangs forever.

Applying your modification does not cure the problem. Applying mine does. Let apply both :-)

@robert-hh
Copy link
Contributor

I can reproduce the lock with a different set-up, using the explicit setting of SPI and Pin. With the default set-up, it does not happen. And if it worked once, it continues to work until the next power cycle. Which indicates a non-initialized variable.

@omogenot
Copy link
Contributor Author

I chose the default WIZNET5K object creation to simplify the test case, but it behaves the same way with "manual" SPI and Pin assignment (with ou without IRQ pin):

from network import WIZNET5K
from machine import SPI, Pin
nic = WIZNET5K(SPI(0, 20_000_000, mosi=Pin(19), miso=Pin(16), sck=Pin(18)), Pin(17), Pin(20), Pin(21))
nic.active(True)
del nic
nic = WIZNET5K()
nic.active(True)

If you apply my patch, does it still hang?

ports/rp2/W5500_EVB_PICO/readme.md: Correct and update WIZNET5K object
usage to comply with current source code.

Signed-off-by: github@mymeterinfo.info
@robert-hh
Copy link
Contributor

robert-hh commented Jul 27, 2022

Yes. I applied the patch and it still hangs. Patched function below. And if I look at wiznet5k_deinit();, it does not seem to be a proper patch.

STATIC void wiznet5k_init(void) {
    // Configure wiznet for raw ethernet frame usage.

    // Configure 16k buffers for fast MACRAW
    uint8_t sn_size[16] = {16, 0, 0, 0, 0, 0, 0, 0, 16, 0, 0, 0, 0, 0, 0, 0};
    ctlwizchip(CW_INIT_WIZCHIP, sn_size);

    if (wiznet5k_obj.use_interrupt) {
        // Enable Wiznet interrupts for socket 0
        wizchip_setinterruptmask(IK_SOCK_0);
        // Enable data receive interrupt
        setSn_IMR(0, Sn_IR_RECV);
        #if _WIZCHIP_ == W5100S
        // Enable interrupt pin
        setMR(MR2_G_IEN);
        #endif

        mp_hal_pin_input(wiznet5k_obj.pin_intn);
        wiznet5k_config_interrupt(true);
    }

    wiznet5k_deinit();

    // Hook the Wiznet into lwIP
    wiznet5k_lwip_init(&wiznet5k_obj);

    netif_set_link_up(&wiznet5k_obj.netif);
    netif_set_up(&wiznet5k_obj.netif);
    // register with network module
    mod_network_register_nic(&wiznet5k_obj); 
}

It fails in wiznet5k_lwip_init() at the call to netif_add().

@robert-hh
Copy link
Contributor

robert-hh commented Jul 27, 2022

My set-up example was wrong. This one works:

import network
from machine import Pin, SPI
spi=SPI(0, miso=Pin(16), mosi=Pin(19), sck=Pin(18))
nic=network.WIZNET5K(spi, Pin(17, Pin.OUT, value=1), Pin(20, Pin.OUT, value=1))
nic.active(1)

With and without IRQ

@robert-hh
Copy link
Contributor

So I dropped the wiznet5k_deinit() call again. It does not make a difference. The standard network tests from the test suite run OK. Summary log below. It is acceptable that some of the tests fail.

./run-multitests.py -i pyb:a0 multi_net/*

20 tests performed
17 tests passed
3 tests failed: multi_net/ssl_cert_rsa.py multi_net/tcp_client_rst.py multi_net/uasyncio_tcp_client_rst.py

./run-multitests.py -i micropython -i pyb:a0 multi_net/*

20 tests performed
16 tests passed
2 tests skipped: multi_net/tcp_client_rst.py multi_net/uasyncio_tcp_client_rst.py
2 tests failed: multi_net/uasyncio_tcp_readall.py multi_net/udp_data.py

python run-tests.py --target pyboard net_inet/*.py net_hosted/*.py

17 tests performed (115 individual testcases)
15 tests passed
2 tests failed: connect_nonblock_xfer test_tls_nonblock

@omogenot
Copy link
Contributor Author

For me the wiznet5k_deinit() call does make a difference. Without it, it bricks the W5500_evb_pico device. So if this does not change anything for you, please leave it for me :-).

@robert-hh
Copy link
Contributor

robert-hh commented Jul 27, 2022

OK. I'll put that back into the code. But I still cannot see why that makes a difference, and that should better be understood.
Edit: I see now: the code blocks if init() is called several times in a row through nic.active(True). Then, there is a undefined state in the network interface data, which is not cleared by just del nic. That happens if you call nic.active(False). So the sequence below works, since with nic.active(False) wiznet5k_deinit() is called.

from network import WIZNET5K
from machine import SPI, Pin
nic = WIZNET5K(SPI(0, 20_000_000, mosi=Pin(19), miso=Pin(16), sck=Pin(18)), Pin(17), Pin(20), Pin(21))
nic.active(True)
nic.active(False)
del nic
nic = WIZNET5K()
nic.active(True)

@omogenot
Copy link
Contributor Author

omogenot commented Aug 1, 2022

@robert-hh
What's the next step for this PR? Will it stay blocked because the first git commit is missing an upper-case A to the verb Add? Shall I discard this PR and forget about it?

@robert-hh
Copy link
Contributor

Normally this is not a show stopper. The PR approval process is very slow, and there are 276 PRs in the queue. That seems impossible to handle.

@dpgeorge
Copy link
Member

Thanks for the contribution, this looks fine. Squashed and merged in 6e51dbd with modified commit message.

@dpgeorge dpgeorge closed this Aug 11, 2022
@omogenot omogenot deleted the W5500_EVB_PICO branch August 14, 2022 13:34
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants