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

Add SpiManager and support W5500 #2306

Merged
merged 11 commits into from
Sep 27, 2024
Merged

Conversation

LennartF22
Copy link
Contributor

@LennartF22 LennartF22 commented Sep 24, 2024

This PR adds a SpiManager library that can be used for dynamically allocating SPI buses for different purposes, (like the two supported RF modules). First, buses that shall be managed by the SpiManager must be registered via the register_bus(...) function. Then, a bus can be claimed by using claim_bus(...) (ESP-IDF numbering) or claim_bus_arduino() (Arduino numbering).

Furthermore, the SpiManager supports "shared" SPI buses (alloc_device(...)), which means that the same SPI peripheral of the ESP32 is used for multiple devices, which might even use completely different pins, making it possible to use more than two SPI devices on the ESP32/ESP32-S2/ESP32-S3 or more than one SPI device on the ESP32-C3. To make use of this feature, the code/library for the device must use the ESP-IDF SPI functions directly instead of the Arduino wrappers, and must allow using a user-supplied spi_device_handle_t. Devices on the same shared SPI bus may or may not use the same set of pins (using the same set of pins has slightly better performance, but performance has been optimized for both cases). All patching/unpatching of pins is handled automatically by the SpiManager by using the pre_cb and post_cb functions of ESP-IDF, meaning that implementation effort on the user side is minimal.

Currently, the nRF24L01+ uses a dedicated SPI bus, as the used library does not support passing an spi_device_handle_t yet. For the CMT2300A, supporting a shared SPI bus was trivial and has no disadvantage for performance, so a shared SPI bus is already used for it. For the future, there are plans to support passing an spi_device_handle_t to the RF24 library for the nRF24L01+ as well, so that it can share the bus with the CMT2300A.

Apart from using a shared SPI bus, several improvements have been implemented for the CMT2300A SPI driver.

This PR also adds support for the W5500 SPI Ethernet MAC+PHY. The W5500 is readily available and also used on the OpenDTU Fusion PoE shield. Adding support for external Ethernet MACs is inevitable for using Ethernet on modern ESP32s (like on the ESP32-S3), as they do not have an internal Ethernet MAC anymore. In the current configuration, it shares the SPI bus peripheral with the CMT2300A. So far, we could not monitor any performance degradation in communication with HMS/HMT inverters.

In comparison to a previous attempt at supporting more SPI devices and the W5500 (see #1200), this version moves complexity from the device drivers to the SpiManager library, so that changes to the drivers are minimal, especially if they already use the ESP-IDF SPI functions directly.

Most importantly, this PR makes it possible to use the nRF24L01+, the CMT2300A and the W5500 simultaneously, without requiring changes to external libraries.

@stefan123t
Copy link

Thanks @LennartF22 for your repeated efforts to solve this abstraction issue ! I have notified the nRF24L01+ library developers by linking the upstream issue to your PR nRF24/RF24#925

@tbnobody
Copy link
Owner

I've already merged it into my local dev branch. I assume you are ok with the GPL-v2 license? Then I would add the license headers to the new files.

@LennartF22
Copy link
Contributor Author

@tbnobody Sure, GPLv2 is fine by me.

@tbnobody
Copy link
Owner

I've just realized that it ends up in a boot loop when flashing the opendtufusionv2_shield environment and no W5500 is connected.

@tbnobody
Copy link
Owner

And it would be nice to have a device profile for the opendtufusionv2_shield board as users just can flash the already existing opendtu-generic_esp32s3.bin file.
I will prepare a section in the already existing opendtu_fusion.json file

@LennartF22
Copy link
Contributor Author

I've just realized that it ends up in a boot loop when flashing the opendtufusionv2_shield environment and no W5500 is connected.

Yes, that was already TODO list. I will add a sanity check that just reads some register to make sure the W5500 is hooked up properly before adding it to the networking stack.

@tbnobody
Copy link
Owner

And as it does not seem to break anything I will most likely release a version including this PR tomorrow evening (if everything works fine in my setup till then :) )

@tbnobody
Copy link
Owner

Yes, that was already TODO list. I will add a sanity check that just reads some register to make sure the W5500 is hooked up properly before adding it to the networking stack.

That would be great. But if possible just share another PR as I have already merged this one.

It also would mean, that only one device profile for the fusion v2 board basic function is required.

@stefan123t
Copy link

stefan123t commented Sep 26, 2024

Would you want to merge the ePaper support in PR #1291 from @dAjaY85 (or the updated PR #1703 from @mtavenrath which superseeded it) as well making the necessary changes / updates, now that the SPIManager will successfully replace the earlier SPIPatcher ?

@LennartF22
Copy link
Contributor Author

And as it does not seem to break anything I will most likely release a version including this PR tomorrow evening (if everything works fine in my setup till then :) )

That‘s great news!

That would be great. But if possible just share another PR as I have already merged this one.

I‘ve implemented the check and will create another PR as soon as this one is in the master.

@tbnobody tbnobody merged commit 326525c into tbnobody:master Sep 27, 2024
@tbnobody
Copy link
Owner

Sorry for the delay.

Not a problem for now but in the arduino core 3 the methods

void tcpipInit();
void add_esp_interface_netif(esp_interface_t interface, esp_netif_t* esp_netif);

do not exist anymore. Seems like the ETH class derives from NetworkInterface

https://github.com/espressif/arduino-esp32/blob/b05f18dad55609ae2a569be81c7535021b880cf3/libraries/Ethernet/src/ETH.h#L120

I stumbled across this because I am currently adapting the code so that it also compiles with Arduino Core 3.x.

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants