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.

Sign up for GitHub

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

Jump to bottom

Introduce SiWx917 Wifi driver #30

Merged
merged 10 commits into from
Sep 19, 2024
Merged

Introduce SiWx917 Wifi driver #30

merged 10 commits into from
Sep 19, 2024

Conversation

jerome-pouiller
Copy link
Contributor

No description provided.

@jerome-pouiller
Copy link
Contributor Author

jerome-pouiller commented Aug 9, 2024 •
edited
Loading

I fixed the compliance with the CI.

v2:

  • Fix indentation
  • Fix compiler warning
  • Improve commit messages

@jerome-pouiller
Copy link
Contributor Author

v3:

  • Compile Wifi sample in our CI
  • Do not enable WIFI by default
  • Since WiFi is not more always enabled, we neither need to enable
    TEST_RANDOM_GENERATOR
  • Do not enforce SYS_CLOCK_TICKS_PER_SEC value
  • Add a FIXME about wifi activation in DT

@jerome-pouiller
Copy link
Contributor Author

v4:

  • Fix missing signed-off

@jhedberg
Copy link
Collaborator

@jerome-pouiller are you still planning to make some updates to this one, or should we just merge it? Until now I've left it to the PR author to merge once it's been approved.

@jhedberg
Copy link
Collaborator

@jerome-pouiller are you still planning to make some updates to this one, or should we just merge it? Until now I've left it to the PR author to merge once it's been approved.

Nevermind - I realized we need to sort out the HAL PR first and once it's merged update the west.yml reference in this one.

@jhedberg
Copy link
Collaborator

Btw, I suppose we may want the same kind of automation as upstream has wrt module tree PR references in west.yml, i.e. that we get a summary as a PR comment here as well as a "Do Not Merge" label which then prevents merging by mistake.

@jerome-pouiller
Copy link
Contributor Author

v4:

  • Add experimental support for connect() (I hesitated to open a new PR for this commit, but since Github do not support PR dependencies, I have added it in this PR)

@jerome-pouiller
Copy link
Contributor Author

jerome-pouiller commented Sep 12, 2024 •
edited
Loading

v5:

  • Add support for socket offloading
  • Add missing stack for NWP
  • Rebase

@jerome-pouiller
Copy link
Contributor Author

v6:

  • Fix bot complains

jhedberg
jhedberg reviewed Sep 18, 2024
View reviewed changes
.github/workflows/build.yml Outdated
@@ -44,3 +44,4 @@ jobs:
EXTRA_TWISTER_FLAGS="--short-build-path -O/tmp/twister-out"
fi
west twister --test sample.basic.helloworld -p siwx917_rb4338a -v --inline-logs $EXTRA_TWISTER_FLAGS
west twister -T ../zephyr/samples -s sample.net.wifi -p siwx917_rb4338a -v --inline-logs -K $EXTRA_TWISTER_FLAGS
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need the -T ../zephyr/samples part (the line above doesn't have it either), and let's be consistent with using either --test or -s but not mix of them.

jhedberg
jhedberg reviewed Sep 18, 2024
View reviewed changes
.github/workflows/upstream-build.yml Outdated
@@ -57,3 +57,4 @@ jobs:
EXTRA_TWISTER_FLAGS="--short-build-path -O/tmp/twister-out"
fi
west twister -T ../zephyr/samples -s sample.basic.helloworld -p siwx917_rb4338a -v --inline-logs $EXTRA_TWISTER_FLAGS
west twister -T ../zephyr/samples -s sample.net.wifi -p siwx917_rb4338a -v --inline-logs -K $EXTRA_TWISTER_FLAGS
Copy link
Collaborator

Choose a reason for hiding this comment

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

The -T ../zephyr/samples is likely unnecessary, but that can be fixed separately as the line above also has the same issue.

jhedberg
jhedberg reviewed Sep 18, 2024
View reviewed changes
soc/silabs/silabs_siwx917/siwg917/soc.c Outdated
* FIXME: Allow to configure size of buffer
*/
uint8_t __aligned(4) siwx917_coprocessor_stack[10 * 1024];
static Z_DECL_ALIGN(struct _isr_list) Z_GENERIC_SECTION(.intList) __used __isr_siwx917_coprocessor_stack_irq = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be split into multiple lines? Compliance is failing due to the long line.

@jerome-pouiller
Copy link
Contributor Author

v7:

  • (try to) fix declaration of __isr_siwx917_coprocessor_stack_irq
  • fix invocation of twister

asmellby
asmellby reviewed Sep 18, 2024
View reviewed changes
drivers/wifi/siwx917/Kconfig.siwx917 Outdated

if WIFI_SIWX917

# WiseConnect create threads with realtime priority. Default (10kHz) clock tick
Copy link
Contributor

Choose a reason for hiding this comment

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

I've looked at implementing the OS timer using sleeptimer backed by sysrtc. This would tick at 1024 Hz, which is the same as FreeRTOS does in Wiseconnect. Is there a reason you needed to reduce it all the way to 100?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote that before you fixed the clock issues. I have just run some tests and it seems not true anymore.

@jerome-pouiller
Copy link
Contributor Author

v8:

  • Relax clock constraints
  • Fix hal_silabs commit id

@jerome-pouiller
soc: silabs: siwg917: Declare memory area for coprocessor stack
8f60710 
With WiseConnect 3.3, Network co-processor (aka NWP aka TA) needs an
area of the M4 processor to store its stack.

This is only required if the NWP is used (WiFi and Bluetooth). However,
since NWP is likely to be used, this patch declare this area
unconditionally.

Note this requirement could be relaxed in the future versions of
WiseConnect.

Signed-off-by: Jérôme Pouiller <[email protected]>
@jerome-pouiller
Copy link
Contributor Author

v9:

  • rebase

@jerome-pouiller
drivers: siwx917_wifi: Introduce infra for SiWx917 Wifi driver
0581cf3 
Introduce an empty shell to store upcoming driver for SiWG917 WiFi.

Note we want to be able to use zephyr/samples/net/wifi. However, since
we working in downstream, we can't add boards/siwx917_rb4338a.overlay.
So, this patch enable WiFi on siwx917_rb4338a by default.

Signed-off-by: Jérôme Pouiller <[email protected]>
@jerome-pouiller
drivers: siwx917_wifi: Add support for iface_status()
587e667 
iface_status() is a mandatory function to be recognized as a true WiFi
driver by Zephyr framework.

Signed-off-by: Jérôme Pouiller <[email protected]>
@jhedberg
Copy link
Collaborator

@jerome-pouiller do you want to try to fix the remaining compliance issues? Those will reoccur if/when this is submitted upstream, since we run the exact same checks, however there does exist the possibility for an override if there's a good justification. This will slow down the merge process however, so it's worth consider to just try to comply anyway.

@jerome-pouiller
drivers: siwx917_wifi: Add support for scan()
9aa86ef 
Scan feature has been tested using samples/net/wifi with the command
"wifi scan".

Signed-off-by: Jérôme Pouiller <[email protected]>
@jerome-pouiller
drivers: siwx917_wifi: Add support for connect()
4a27833 
This patch as been successfully tested in unencrypted, WPA2 and
WPA3-transitional (= SAE without mandatory MFP). I have had some issues
with networks requiring MFP.

This patch does not implement EAP and WEP authentication. This mainly
because Zephyr does not describe the API to pass the credential.

This patch does not support static IP configuration. However DHCPv4
works properly.

On IPv6, I have not been able to configure IPv6 using SLAAC (it is not
clear if WiseConnect 3.3 provide the feature).

Signed-off-by: Jérôme Pouiller <[email protected]>
@jerome-pouiller
drivers: siwx917_wifi: Add support for struct net_offload
e34d9ee 
This implementation allows to take advantage of the specific features
provided by the 917 (power consumption, speed, validation...).

Some notable features are not available with this interface:
  - It seems Zephyr does not provide API to offload multicast membership
    management. User should be to directly call WiseConnect APIs
  - Support for ICMP frames is difficult. Note that WiseConnect
    automatically answer to ping request. It is just not possible to
    send ping requests and receive ping responses.
  - Zephyr and WiseConnect both support TLS offloading. However this
    patch does not implement it.
  - Reentrancy in the WiseConnect side is uncertain.

This implementation has been tested with samples/net/wifi/ (which relies
on subsys/net/lib/shell).

Signed-off-by: Jérôme Pouiller <[email protected]>
@jerome-pouiller
drivers: siwx917_wifi: Add support for timeout in recv()
81c6f06 
Zephyr recv API requires to able to call the callback() asynchronously.

This implementation is more intrusive than I would like. Especially,
simultaneous access to fields fds_* does not seems bullet proof.
Unfortunately, with the use of callbacks, the risk of deadlocks looks
greater than the risk of data corruption.

Signed-off-by: Jérôme Pouiller <[email protected]>
@jerome-pouiller
drivers: siwx917_wifi: Report RSSI in iface_status()
305613a 
RSSI is easy to report.

Signed-off-by: Jérôme Pouiller <[email protected]>
@jerome-pouiller
workflows: Also check samples/net/wifi
97e6df8 
Board siwx917_rb4338a now supports WiFi. Let's ensure nobody is broken
it.

Signed-off-by: Jérôme Pouiller <[email protected]>
@jerome-pouiller
Copy link
Contributor Author

v10:

  • reword commit logs

@jhedberg
github: workflows: Update compliance to match what upstream does
404b58c 
Downgrade ClangFormat errors to warnings.

Signed-off-by: Johan Hedberg <[email protected]>
@jhedberg jhedberg merged commit 892c436 into SiliconLabsSoftware:main Sep 19, 2024
4 checks passed
@jerome-pouiller jerome-pouiller mentioned this pull request Sep 19, 2024
Merged
@jerome-pouiller jerome-pouiller deleted the siwx917 branch September 20, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asmellby asmellby asmellby left review comments

@jhedberg jhedberg jhedberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jerome-pouiller @jhedberg @asmellby