esp: Wi-Fi support #861
esp: Wi-Fi support #861
Conversation
- Fix idle task stack overflow - Add alignment to data sections - Upgrade `radio/timer.zig` to use a separate task - Add max value to semaphore
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Lint Results
Found 9 issues on changed lines in 9 files:
- examples/espressif/esp/src/tcp_server.zig: 1 issue
- examples/raspberrypi/rp2xxx/src/net/lwip/net.zig: 1 issue
- port/espressif/esp/src/hal/radio/osi.zig: 1 issue
- port/espressif/esp/src/hal/system.zig: 1 issue
- port/raspberrypi/rp2xxx/src/hal/usb.zig: 1 issue
- port/stmicro/stm32/src/hals/STM32F103/adc.zig: 1 issue
- port/stmicro/stm32/src/hals/STM32F103/i2c.zig: 1 issue
- port/stmicro/stm32/src/hals/STM32F103/rcc.zig: 1 issue
- port/stmicro/stm32/src/hals/common/i2c_v2.zig: 1 issue
| const maybe_cs = self.critical_section; | ||
| self.critical_section = null; | ||
| if (maybe_cs) |cs| { |
| var ip_ready_semaphore: rtos.Semaphore = .init(0, 1); | ||
| var ip: lwip.c.ip_addr_t = undefined; | ||
|
|
||
| extern fn netconn_new_with_proto_and_callback(t: lwip.c.enum_netconn_type, proto: lwip.c.u8_t, callback: ?*const anyopaque) [*c]lwip.c.struct_netconn; |
There was a problem hiding this comment.
translate c output for lwip has some issues
| } | ||
| }; | ||
|
|
||
| pub fn expect_handler(comptime int: Interrupt, comptime expected_handler: InterruptHandler) void { |
There was a problem hiding this comment.
This is an interesting pattern to use in other hals as well
| @export(&strlen, .{ .name = "strlen", .linkage = .weak }); | ||
| @export(&strnlen, .{ .name = "strnlen", .linkage = .weak }); | ||
| @export(&strrchr, .{ .name = "strrchr", .linkage = .weak }); | ||
|
|
||
| @export(&__assert_func, .{ .name = "__assert_func", .linkage = .weak }); | ||
|
|
||
| @export(&malloc, .{ .name = "malloc", .linkage = .weak }); | ||
| @export(&calloc, .{ .name = "calloc", .linkage = .weak }); | ||
| @export(&free, .{ .name = "free", .linkage = .weak }); | ||
| @export(&puts, .{ .name = "puts", .linkage = .weak }); | ||
|
|
||
| @export(&gettimeofday, .{ .name = "gettimeofday", .linkage = .weak }); | ||
| @export(&sleep, .{ .name = "sleep", .linkage = .weak }); | ||
| @export(&usleep, .{ .name = "usleep", .linkage = .weak }); | ||
| @export(&vTaskDelay, .{ .name = "vTaskDelay", .linkage = .weak }); |
There was a problem hiding this comment.
These can be overwritten by the user. They are provided in case there is no available implementation. But they are required for esp_wifi drivers to work
|
|
||
| .data : | ||
| { | ||
| . = ALIGN(16); |
There was a problem hiding this comment.
I also encountered some alignment caused crashes.
There was a problem hiding this comment.
I get littering the linker with align directives, but do you actually know for sure that these need align 16, or are you just going this high to be safe?
There was a problem hiding this comment.
I used 16 because I got a warning from the linker that .data should be aligned to 16 bytes.
github-actions
bot
dismissed
their stale review
Updating with new lint results
github-actions
bot
dismissed
their stale review
Updating with new lint results
|
I managed to crash the formatter :) The problematic file is |
github-actions
bot
dismissed
their stale review
Updating with new lint results
github-actions
bot
dismissed
their stale review
Updating with new lint results
|
|
||
| .data : | ||
| { | ||
| . = ALIGN(16); |
There was a problem hiding this comment.
I get littering the linker with align directives, but do you actually know for sure that these need align 16, or are you just going this high to be safe?
| const systimer = @import("systimer.zig"); | ||
|
|
||
| // How it works? | ||
| // For simple task to task context switches, only necessary registers are |
There was a problem hiding this comment.
Only the registers that the compiler wants to save. This happens through the clobbers from the inline asm context_switch.
There was a problem hiding this comment.
Yeah I mean to say that you should be more specific in the comment. It's defined by the ABI?
| // For simple task to task context switches, only necessary registers are | ||
| // saved. But if a higher priority task becomes available during the handling | ||
| // of an interrupt, a task switch is forced by saving the entire state of the | ||
| // task on the stack. What is interresting is that the two context switches are |
There was a problem hiding this comment.
| // task on the stack. What is interresting is that the two context switches are | |
| // task on the stack. What is interesting is that the two context switches are |
| microzig.cpu.interrupt.enable(rtos_options.yield_interrupt); | ||
|
|
||
| // unit0 is already enabled as it is used by `hal.time`. | ||
| if (rtos_options.systimer_unit != .unit0) { |
There was a problem hiding this comment.
is it OK to do this? should it be a compile error?
There was a problem hiding this comment.
If the user overrides the systimer_unit to be something other than the unit used by esp.time which is already enabled, we should enable it. Otherwise, enabling the unit is unnecessary.
| while (!timeout.is_reached()) { | ||
| yield(.{ .wait = .{ | ||
| .timeout = timeout, | ||
| } }); |
There was a problem hiding this comment.
I am betting zig fmt will complain about this
| } | ||
| }; | ||
|
|
||
| pub const TypeErasedQueue = struct { |
There was a problem hiding this comment.
this and the normal queue could maybe go into some utility library? Just something about the rtos state would have to be generalized.
If you think it would fit, a TODO is fine by me.
There was a problem hiding this comment.
Well this queue sort of comes with the rtos. The implementation is made specifically for it. Though, it would be nice to allow the user to swap the RTOS for wifi.
There was a problem hiding this comment.
ah ok, that's fine by me, then
| }, | ||
| }; | ||
|
|
||
| const SSID = "SSID"; |
There was a problem hiding this comment.
Maybe add a comment here and a compile error if it's not set
There was a problem hiding this comment.
I'll add a comment. If we add a compile error, wouldn't CI fail?
| // TODO: config | ||
| const coex_enabled: bool = false; | ||
|
|
||
| pub var gpa: std.mem.Allocator = undefined; |
There was a problem hiding this comment.
nit but GPA implies general purpose allocator. Do they normally call it GPA unless it's actually that (or the debug allocator now)?
There was a problem hiding this comment.
Afaik, it is a good practice to name allocators gpa or arena to imply what kind of allocator do you need to provide. If it is an arena, you expect it to leak memory.
| tim.deadline = .init_relative(get_time_since_boot(), duration); | ||
| tim.periodic = if (repeat) duration else null; | ||
| } else { | ||
| log.warn("timer not found based on ets_timer", .{}); |
There was a problem hiding this comment.
Can we actually survive these? Should we be panicking if it's game over here rather than failing later on?
| if (find(ets_timer)) |tim| { | ||
| tim.deadline = .no_deadline; | ||
| } else { | ||
| log.warn("timer not found based on ets_timer", .{}); |
There was a problem hiding this comment.
For better debugging, would be nice to print which function failed (arm vs done vs disarm)
| }, | ||
| }; | ||
|
|
||
| const SSID = "SSID"; |
| } | ||
| }; | ||
|
|
||
| pub const TypeErasedQueue = struct { |
There was a problem hiding this comment.
ah ok, that's fine by me, then
| const systimer = @import("systimer.zig"); | ||
|
|
||
| // How it works? | ||
| // For simple task to task context switches, only necessary registers are |
There was a problem hiding this comment.
Yeah I mean to say that you should be more specific in the comment. It's defined by the ABI?
No description provided.