extmod: Move modnetwork, modusocket to extmod. #7698
Conversation
5749a4e to
c8a270a
Compare
Codecov Report
@@ Coverage Diff @@
## master #7698 +/- ##
=======================================
Coverage 98.25% 98.25%
=======================================
Files 154 154
Lines 20071 20071
=======================================
Hits 19721 19721
Misses 350 350
Continue to review full report at Codecov.
|
804f837 to
496cbba
Compare
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 FWIW I have a PYBD_SF2 and I tested CYW43 with these changes and I can still run a simple network scan. |
d389501 to
b17c427
Compare
|
@dpgeorge Can this be processed first/next because I need to rebase my other PRs accordingly. |
|
@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. |
|
Sorry for the delay, I'll get to this next. |
|
Thanks. Please take your time. I want to wait anyhow until PR #7767 is done and I can rebase on that state. |
Yes,
Yes, please rename it to
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). |
|
Also, Edit: we may eventually want to move esp32 and unix's modusocket.c to extmod, called, eg, |
b17c427 to
b7db26b
Compare
b7db26b to
f630cdc
Compare
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;
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 ? |
f630cdc to
3317a7f
Compare
|
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 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; |
I think because the C standard does not like anonymous structs/unions (???).
The point of the union is just to save RAM, that some NIC sockets might want to just use
Yes that makes sense. |
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.
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.
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 |
Ok. We support C99 so cannot use anonymous structs/unions.
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.
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. |
Okay I made all the required changes, it's done now. |
|
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! |
For discussion and review, related PR see:
I suggest that network interfaces be defined in mpconfigport.h making modnetwork completely generic.