8000 extmod: Move modnetwork, modusocket to extmod. by iabdalkader · Pull Request #7698 · micropython/micropython · GitHub
[go: up one dir, main page]

Skip to content

extmod: Move modnetwork, modusocket to extmod. #7698

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

Closed
wants to merge 5 commits into from

Conversation

iabdalkader
Copy link
Contributor
@iabdalkader iabdalkader commented Aug 22, 2021

For discussion and review, related PR see:

I suggest that network interfaces be defined in mpconfigport.h making modnetwork completely generic.

@codecov-commenter
Copy link
codecov-commenter commented Aug 22, 2021

Codecov Report

Merging #7698 (b17c427) into master (b51e7e9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7698   +/-   ##
=======================================
  Coverage   98.25%   98.25%           
=======================================
  Files         154      154           
  Lines       20071    20071           
=======================================
  Hits        19721    19721           
  Misses        350      350           
Impacted Files Coverage Δ
py/bc.c 88.65% <0.00%> (-1.04%) ⬇️
py/parse.c 98.97% <0.00%> (-0.21%) ⬇️
py/runtime.c 99.39% <0.00%> (+0.15%) ⬆️
py/obj.c 97.61% <0.00%> (+0.39%) ⬆️

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 b51e7e9...b17c427. Read the comment docs.

@dpgeorge dpgeorge added the extmod Relates to extmod/ directory in source label Aug 24, 2021
@dpgeorge
Copy link
Member

making modnetwork completely generic

Yes that's a good goal.

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Aug 24, 2021

making modnetwork completely generic

Yes that's a good goal.

Done b17c427 is the naming okay ?

Also should mplwipport.c be named mpnetworkport.c instead ?

EDIT: That said and done, I also have a network WiFi driver similar to Nina-W10 for WINC1500 that I was considering sending upstream later. To work properly this driver must have a 1.5K buffer per socket added in the socket state. I was thinking if at some point it was sent upstream, how would the socket buffer be added to the socket state in modnetwork.h while still keeping it generic ? Should this last change be reverted and just let modnetwork be aware of all the supported possible network interfaces ? It would be nice to have a way to specify what NICs are bound to WAN/LAN though.

EDIT2: Or maybe MICROPY_PY_USOCKET_EXTENDED_STATE should be used to add additional members to socket_obj_t.

FWIW I have a PYBD_SF2 and I tested CYW43 with these changes and I can still run a simple network scan.

@iabdalkader iabdalkader force-pushed the modnetwork_extmod branch 2 times, most recently from d389501 to b17c427 Compare August 24, 2021 23:46
@iabdalkader
Copy link
Contributor Author

@dpgeorge Can this be processed first/next because I need to rebase my other PRs accordingly.

@robert-hh
Copy link
Contributor
robert-hh commented Sep 6, 2021

@dpgeorge Hi Damien. Are you going to integrate that PR anytime soon? If yes, I'll wait with the PR for Ethernet support for mimxrt until this is done. Otherwise, the Ethernet support is ready since 2 Months and working fine in a test set-up since then.

@iabdalkader iabdalkader changed the title Move modnetwork, modusocket to extmod extmod: Move modnetwork, modusocket to extmod. Sep 12, 2021
@dpgeorge
Copy link
Member

Sorry for the delay, I'll get to this next.

@robert-hh
Copy link
Contributor

Thanks. Please take your time. I want to wait anyhow until PR #7767 is done and I can rebase on that state.

@dpgeorge
Copy link
Member

Done b17c427 is the naming okay ?

Yes, MICROPY_PORT_NETWORK_INTERFACES is a good name.

Also should mplwipport.c be named mpnetworkport.c instead ?

Yes, please rename it to mpnetworkport.c.

Or maybe MICROPY_PY_USOCKET_EXTENDED_STATE should be used to add additional members to socket_obj_t

It might make sense to have a generic pointer to extra state, eg:

typedef struct _mod_network_socket_obj_t {
    mp_obj_base_t base;
    mp_obj_t nic;
    mod_network_nic_type_t *nic_type;
    union {
        struct {
            uint8_t domain;
            uint8_t type;
            int8_t fileno;
            uint8_t bound;
        } u_param;
        mp_uint_t u_state;
    };
    // Extended socket state for sockets that need it.
    void *state;
} mod_network_socket_obj_t;

Then the particular NIC socket can allocate as needed (eg if there's a WINC1500 and WIZNET connected, then they can each allocate as much as they need when bound to the NIC).

@dpgeorge
Copy link
Member
dpgeorge commented Sep 14, 2021

Also, extmod/modusocket.c should be renamed to reflect the implementation that it provides, something like extmod/modusocket_nic.c; maybe you can think of a better name. See eg modussl_mbedtls.c and modussl_axtls.c.

Edit: we may eventually want to move esp32 and unix's modusocket.c to extmod, called, eg, extmod/modusocket_posix.c.

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Sep 14, 2021
  • Used mp_stream_read/write/readinto/readline
  • Renamed mplwipport.c to mpnetworkport.c
  • Add generic state pointer (configurable) and timeout to socket object.

It might make sense to have a generic pointer to extra state, eg:

        mp_uint_t u_state;
    };
    // Extended socket state for sockets that need it.
    void *state;
} mod_network_socket_obj_t;

Since state isn't actually needed right now, I made that configurable and instead I put timeout there, can we keep that ? It can be used for unix sockets blocking too.

        mp_uint_t u_state;
    };
    uint16_t timeout;
    #if MICROPY_PY_USOCKET_EXTENDED_STATE
    // Extended socket state for ports that need it.
    void *state;
    #endif
} mod_network_socket_obj_t;

Also, extmod/modusocket.c should be renamed to reflect the implementation that it provides, something like extmod/modusocket_nic.c; maybe you can think of a better name. See eg modussl_mbedtls.c and modussl_axtls.c.
Edit: we may eventually want to move esp32 and unix's modusocket.c to extmod, called, eg, extmod/modusocket_posix.c.

We can just keep it for now and when/if unix/esp32 sockets needs to be shared with other ports, then we can move and rename them. Or do you prefer it gets renamed right now ? modusocket_nic.c is fine, also modusocket_wrapper.c ?

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Sep 14, 2021

On a related note, I wanted to discuss u_param. Why is it a named struct ? If it's anonymous, fields can be accessed like this socket->domain instead of socket->u_param.domain just less typing and u_state allows setting u_param. Additionally, fileno is a singed byte, looking at NICs/ports the fd can go up to 65K. Therefo 8000 re I suggest the following:

    union {
        struct {
            uint32_t domain      :6;
            uint32_t type        :6;
            int32_t fileno       :16;
            uint32_t bound       :1;
            uint32_t /*reserved*/:3;
        };
        uint32_t u_state;
    };

NOTE: if u_state is to be used to set the whole struct, we must account for byte order. More specifically, the complete socket struct:

typedef struct _mod_network_socket_obj_t {
    mp_obj_base_t base;
    mp_obj_t nic;
    mod_network_nic_type_t *nic_type;
    union {
        struct {
        #if defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
            uint32_t domain      :6;
            uint32_t type        :6;
            int32_t fileno       :16;
            uint32_t bound       :1;
            uint32_t /*reserved*/:3;
        #elif defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__)
            uint32_t /*reserved*/:3;
            uint32_t bound       :1;
            int32_t fileno       :16;
            uint32_t type        :6;
            uint32_t domain      :6;
        #else
        #error "Byte order is not defined."
        #endif
        };
        mp_uint_t u_state;
    };
    #if MICROPY_PY_USOCKET_EXTENDED_STATE
    // Extended socket state for NICs/ports that need it.
    int32_t timeout;
    void *state;
    #endif
} mod_network_socket_obj_t;

@dpgeorge
Copy link
Member

On a related note, I wanted to discuss u_param. Why is it a named struct ?

I think because the C standard does not like anonymous structs/unions (???).

if u_state is to be used to set the whole struct, we must account for byte order.

The point of the union is just to save RAM, that some NIC sockets might want to just use u_state to store its state, rather than the u_param entries. The CC3000 driver does this (we could eventually deprecate that driver, it's very old and probably unused).

Therefore I suggest the following: ...

Yes that makes sense.

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Sep 14, 2021

I think because the C standard does not like anonymous structs/unions (???).

I'm not sure but it seems they were always supported but only made standard in C11 see this (search for anonymous), sorry can't find the source.

The CC3000 driver does this (we could eventually deprecate that driver, it's very old and probably unused).

I looked at that driver and it uses u_state to set the fd, seems it needs more than int8_t for fd, which the bit field would fix.

Yes that makes sense.

Do you want to remove u_param and make the struct anonymous as suggested above in this PR ? Or a new PR, or just not change it at all ?

Alternatively we can just make fileno wider, although I prefer the bit-fields because it leaves some reserved flags.

    union {
        struct {
            uint8_t domain;
            uint8_t type;
            int16_t fileno;
        };
        mp_uint_t u_state;
    };
    #if MICROPY_PY_USOCKET_EXTENDED_STATE
    // Extended socket state for NICs/ports that need it.
    int16_t timeout;
    uint8_t bound;
    void *state;
    #endif

@dpgeorge
Copy link
Member

I'm not sure but it seems they were always supported but only made standard in C11

Ok. We support C99 so cannot use anonymous structs/unions.

I looked at that driver and it uses u_state to set the fd, seems it needs more than int8_t for fd, which the bit field would fix.

I'm not sure exactly what the maximum size of fd's are returned from the low-level CC3000 socket driver. That would need to be checked.

Do you want to remove u_param and make the struct anonymous as suggested above in this PR ? Or a new PR, or just not change it at all ?

It would be good to remove the union, which might be possible if fileno width is increased to 16 bits.

We cannot use anonymous structs.

Regardless, let's do that as a separate PR so this one can be merged.

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Sep 14, 2021

Regardless, let's do that as a separate PR so this one can be merged.

Okay I made all the required changes, it's done now.

@dpgeorge
Copy link
Member

Merged in 7aab0dc through 4dba04a, with follow up in a34d43b

Thanks @iabdalkader for the work on this, it's a nice clean up and generalisation!

@dpgeorge dpgeorge closed this Sep 14, 2021
@iabdalkader iabdalkader deleted the modnetwork_extmod branch September 14, 2021 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extmod Relates to extmod/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0