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 Support for W5500 SPI Ethernet Controller #1200

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

LennartF22
Copy link

In order to use Ethernet on newer ESP32 versions like the ESP32-S3, which don't have an Ethernet MAC included anymore, Ethernet over SPI has to be used. In this PR, the W5500 Ethernet controller can be configured in the pin_mapping.json or via build flags, similar to how Ethernet over RMII was (and still is) supported before.

Unfortunately, both externally available SPI master drivers of the ESP32 are already in use when the nRF24 and CMT2300A are configured. Although both RF modules could theoretically run on the same SPI bus (with shared data and clock pins) without issues, support for having the two RF modules on different physicals pins of the ESP32 must be maintained for obvious reasons.

To still support SPI Ethernet, this PR implements a wrapper for SPI transactions initiated by the RF module drivers, which reconfigures the signals between the two physical sets of GPIO pins and only one, shared SPI peripheral automatically when needed. The switching/reconfiguration process has been heavily optimized, so that this change should not have any negative impact on the performance of OpenDTU.

As the RF24 library currently only supports directly using the Arduino SPI functions, it had to be forked. In the future, we could try to create a PR to bring the added functionality of the RF24 library to the original repository, or we could just continue using the fork, as the RF24 library probably won't have relevant changes for OpenDTU/ESP32 anyway.

The code has already successfully been tested by multiple users of the OpenDTU Fusion (together with the new OpenDTU Fusion Shield).

@tbnobody
Copy link
Owner

tbnobody commented Aug 2, 2023

Could you please cleanup your commit history? I see a lot of temporary commits etc. Its not required to put all into one commit.

@LennartF22
Copy link
Author

Should be better now.

@markusdd
Copy link
Contributor

markusdd commented Aug 2, 2023

comment from my end:

  1. thanks so much Lennart for driving this
  2. have we ensured this also at least compiles for S2/C3 targets?

I think for plain ESP32 this is not so important because there you have the MAC boards like OLIMEX but C3 and S2 also come without the MAC and will rely on SPI. W5500 breakouts without PoE can be had rather cheap, so I guess this support will also spark some ideas from other people.

@LennartF22
Copy link
Author

It should now successfully compile for all platforms 😅

@D3R-ST3FAN
Copy link

are there still any issues open?
Or what is the reason this can't be merged yet?

@markusdd
Copy link
Contributor

I think it just has not been revisited yet.
But I would also appreciate the merge because without it we cannot make or distribute the PoE/Ethernet hat for the Fusion board and S3 Ethernet does not work.

@markusdd
Copy link
Contributor

markusdd commented Sep 8, 2023

@tbnobody do you have anything else that needs to be adressed?
We are getting quite some requests for Ethernet and the PoE Shield but without this PR I do not want to pull the trigger on any PCB production.

@markusdd
Copy link
Contributor

unfortunately this has now rotten so that there are conflicts. @LennartF22 are they easy to resolve?

If there are no other open items I would urge to clear up remaining issues and merge this.
We are currently holding back any production run of the PoE/W5500 extension boards as this is not merged, but we have already more than 20 people lined up who would like to use this.

@LennartF22
Copy link
Author

@markusdd The merge conflicts are negligible and easy to fix, but I won't fix them right now, because the platformio.ini might very well change again in master before the merge, which could trigger a new merge conflict.

@tbnobody It would probably be good if I reimplemented my changes to the RF24 library in a clean way, so that my changes could eventually be merged into RF24 library's master branch. However, I will only make this effort if I get your commitment to merge this PR here at some point.

@stefan123t
Copy link

stefan123t commented Oct 25, 2023

@LennartF22 I opened a question on how to get the changes for the virtual HAL into RF24 library upstream.
Kindly check the nRF24/RF24#925
Ah you already noticed and answered there. Thanks!

@3PrintD-Solution
Copy link

@markusdd The merge conflicts are negligible and easy to fix, but I won't fix them right now, because the might very well change again in before the merge, which could trigger a new merge conflict.platformio.ini``master

@tbnobody It would probably be good if I reimplemented my changes to the RF24 library in a clean way, so that my changes could eventually be merged into RF24 library's branch. However, I will only make this effort if I get your commitment to merge this PR here at some point.master

@LennartF22 Great, I see you haven't been accepted yet

@tbnobody What is the current state of play now for the merge?

@D3R-ST3FAN
Copy link

is this going to be integrated any time soon ?

@stefan123t
Copy link

stefan123t commented Dec 19, 2023

@LennartF22 besides the straight forward way of adding a custom device / hardware "utility driver" to the RF24 library upstream, we could also add your changes to the RF24 library eg as custom_patches=opendtufusion_spi_hal/rf24.patch to the OpenDTU platformio.ini in the [opendtufusion_v2] section only.

@tbnobody could you answer @LennartF22 's question about this PR or tell us your thoughts about supporting the OpenDTU Fusion boards with its Power-over-Ethernet add-on adding such an out-of-the-box build ?

@markusdd
Copy link
Contributor

markusdd commented Dec 19, 2023

I should add that this is not only about ethernet, this also is the basis for solving the issue of running 2 RFs and e.g. an additional ePaper display in a stable fashion.

@gitisgreat2023
Copy link

@LennartF22 is it possible to be able the Huawei 4850G2 in combination with the CMT2300A? It is a software issue, right? Could it be solved in a similar way this ethernet issue was solved?

@gitisgreat2023
Copy link

@markusdd
also for running the CMT2300A RF in combination with a Huawei 4850G2? Would that be possible then as well?

That would be awesome, adding support for the HMS/HMT with Huawei would be great. Currently only HM can be combined with the Huawei (i.e. max HM 1500 instead of for example combing the HMT-2250 with the Huawei).

I should add that this is not only about ethernet, this also is the basis for solving the issue of running 2 RFs and e.g. an additional ePaper display in a stable fashion.

@markusdd
Copy link
Contributor

Hi. No. What you are asking for is the reverse.
What we are trying to accomplish is to use 2 or 3 external devices over one internal SPI bus.

What you want is one external device to be driven by 2 different software drivers. This is certainly possible, but completely unrelated to what we do here.

Only connection is that once this here would finally get a review and maybe approval, the path to doing what you are asking for would become easier.

@gitisgreat2023
Copy link

Hi. I'm looking for a solution that a CMT2300A can be used together with the Huawei and Pylontech. The Pylontech is covered by the ESP32 CAN controller. So also in my case two devices need to be controlled over one SPI bus (HMT/HMS and Huawei).
Isn't the same case as here with the ethernet controller?

The case that both the CMT2300A and NRF24L01+ are needed are currently not in focus, as the power limiting function only controls one Hoymiles.

@markusdd
Copy link
Contributor

Is the Huawei using 3-wire SPI? That sounds like some long cables. Or is it controlled via the CMT RF?

@LennartF22
Copy link
Author

@gitisgreat2023 @markusdd

TL;DR Yes, this would probably also fix your issue, but there are better alternatives (like making use of the ESP32's internal CAN controller).

As far as I understood, the Huawei 4850G2 can be controlled via CAN bus, and the current implementation uses an external CAN controller + transceiver (probably MCP2515), and is not using the internal CAN controller of the ESP32 for some reason. The best solution would be to just use the internal CAN controller of the ESP32, which just requires a CAN transceiver to be hooked up to the ESP32, but still requires some code changes.

Using the CMT2300A and an external CAN controller via SPI should not be a problem at all though, because we can give both of them their own SPI bus, as long as there is no nRF24L01+ on top of that. If that configuration does not work, the problem is most probably that the SPI busses are assigned statically (for example the CMT2300A might always uses bus 2, and the CAN controller might always try to use bus 2, too). This should be a rather easy software fix.

It gets more problematic when there are more than two SPI devices. Of course hooking up more than one device to the same SPI bus is possible, but it requires much attention due to concurrency and interrupts. It becomes even more complicated when a three wire and normal SPI device need to operate on the same SPI bus.

In this PR, I implemented sharing of an SPI bus between the nRF and the CMT, which means that one SPI bus is always free, no matter which RF modules are used (it is even possible to use both simultaneously). This leaves the second SPI bus free for many other applications: for example SPI Ethernet controllers, CAN controllers and displays. Although this would solve the issue with using the CMT2300A and an external CAN controller simultaneously, it is not the prefered solution, as you then cannot use stuff like SPI Ethernet at the same time without major software headaches, while it can be solved in a much simpler way (alluding to the internal CAN controller of the ESP32).

@gitisgreat2023
Copy link

gitisgreat2023 commented Jan 24, 2024

@LennartF22 @markusdd
Thanks for you elaboration. In the OpenDTU-OnBattery PCB solutions as battery Pylontechs are used (note the JK-BMS is now also supported). The Pylontech is using the internal EPS32 CAN controller.

OK, in theory the RS485 could be used for the Pylontech, there is even code on it.

@gitisgreat2023
Copy link

@LennartF22

"If that configuration does not work, the problem is most probably that the SPI busses are assigned statically (for example the CMT2300A might always uses bus 2, and the CAN controller might always try to use bus 2, too). This should be a rather easy software fix."

I fully agree that that software solution would be ideal:
SPI 1: CAN of Huawei, or ethernet controller
SPI 2: CMT2300A (or NRF24l01+, I understand both can be hardware present and by software only one might then be active at at time, to avoid SPI bus sharing with all the headaches)
CAN of ESP32: Pylontech

I don't understand how we achieve this "rather easy software fix". Which steps do we need to take to achieve that?
This PR needs to be accepted? Do the code proposal for the RF24 also need to be accepted?

@tbnobody
Copy link
Owner

tbnobody commented Feb 3, 2024

I've rebased the source and made several changes already:

  • 98eaa510 2024-02-03 Remove no more required defines
  • daee8154 2024-02-03 Apply automatic code formatting
  • 099aa08f 2024-02-03 Implement ETHSPI as separate library
  • 322adc11 2024-02-03 Implement for the CMT2300A class the same interface as for the RF24 class
  • 575b360e 2024-02-03 Prefix private variables with underscore
  • afa6c7e5 2024-02-03 Move nrf_hal to separate folder
  • e94c3e26 2024-02-03 Renamed HoymilesSpiPatcher to SpiPatcher
  • 3c5114cd 2024-02-03 Apply automatic code formatting
  • 3f6c7709 2024-02-03 Implement SpiPatcher as separate library

Currently figured out the following problems when compiling with generic_esp32c3.

lib/ETHSPI/src/ETHSPI.cpp: In member function 'void ETHSPIClass::begin(int8_t, int8_t, int8_t, int8_t, int8_t, int8_t)':
lib/ETHSPI/src/ETHSPI.cpp:48:40: error: 'SPI3_HOST' was not declared in this scope
     ESP_ERROR_CHECK(spi_bus_initialize(SPI3_HOST, &buscfg, SPI_DMA_CH_AUTO));

That's because espc3 has less SPI hosts.

@markusdd wrote here

I should add that this is not only about ethernet, this also is the basis for solving the issue of running 2 RFs and e.g. an additional ePaper display in a stable fashion.

That's true as long as a patched version of a display library is used right? Because it must also support the handling with SpiPatcher. My fear is to maintain a whole bunch of librarys myself and upgrade it's versions.

@LennartF22
Copy link
Author

@tbnobody That's good to hear!

I definitely understand that you don't want to maintain several additional, external libraries, as that sort of defeats the purpose of having external libraries in the first place :)

That's why we already contacted the RF24 maintainers on how they would go about adding support for our needs in a proper fashion, and quickly found a good solution, but nobody has implemented it yet. I haven't been particularly motivated myself so far to implement it, because this PR was seemingly not going anywhere, but now I will try to take care of it as soon as I find some spare time.

That's true as long as a patched version of a display library is used right? Because it must also support the handling with SpiPatcher. My fear is to maintain a whole bunch of librarys myself and upgrade it's versions.

Yes and no. I guess you would already profit from the two RF modules only using one SPI bus, because as long as you don't need Ethernet on lets say an ESP32-S3, you now have a dedicated SPI bus to spare, so that available libraries can be used off-the-shelf.

On top of that, I think there even was a PR with a modified library for some SPI display that used bit banging to get around the SPI bus issues already. I don't know about the fate of that PR, but using the SPI patcher over the bit banging would be a prettier solution, if some modifications need to be done to the library anyways, although some mechanism would be needed to avoid the display driver claiming the SPI bus for too long transfers.

Currently figured out the following problems when compiling with generic_esp32c3.

So currently, only using one RF module with the ESP32-C3 is supported? I think nothing speaks against using SPI2_HOST (instead of SPI3_HOST) for the RF modules on all platforms, and - if applicable - using SPI3_HOST for additional hardware.

@tbnobody
Copy link
Owner

tbnobody commented Feb 6, 2024

So currently, only using one RF module with the ESP32-C3 is supported? I think nothing speaks against using SPI2_HOST (instead of SPI3_HOST) for the RF modules on all platforms, and - if applicable - using SPI3_HOST for additional hardware.

The problem is indeed the ETHSPI class which uses SPI3_HOST.

The SpiPatcher which uses SPI2

#define HOYMILES_HOST_DEVICE SPI2_HOST

works without problems.

@LennartF22
Copy link
Author

LennartF22 commented Feb 6, 2024

Oh I see, I didn't read properly 😅

Ideally, we would not build the ETHSPI class for the ESP32-C3 target at all, but that obviously also requires some macros wherever the ETHSPI object is called.

We should also be able to use a hack like this, as SPI Ethernet on the ESP32-C3 won't/cannot be used anyway:

#ifdef SPI3_HOST
#define ETH_HOST_DEVICE SPI3_HOST
#else
#define ETH_HOST_DEVICE SPI2_HOST
#endif

@D3R-ST3FAN
Copy link

Is this still being worked on?

@Wookbert
Copy link

Wookbert commented Jun 1, 2024

+1 ... kind of. I – like so many others – am interested in seeing the W5500 being supported by OpenDTU, but can thoroughly understand tbnobody’s dislike for a half-baked, hacked solution which requires continuous attention in order to keep things properly working.

@markusdd
Copy link
Contributor

markusdd commented Jun 1, 2024

I wouldn't agree that it is hacked tbh.

Frankly some of this is actually a necessary clean-up or improvement of how spi devices are handled in general, the real issue is that Espressif has made our life very hard by changing around the SPI port nomenclature.

My issue is that I cannot get ANY feedback what the holdup is or what we can do about it.
There is a large amount of people waiting for this and we have halted PoE shield production for more than half a year now. We have now ordered more because at least ahoy now has good mainline support for this.

@Wookbert
Copy link

Wookbert commented Jun 1, 2024

Being able to attach both an ePaper display and the W5500 is also a must IMO. That’s what SPI is made for.

@markusdd
Copy link
Contributor

markusdd commented Jun 1, 2024

While I understand that (and it is possible), I still want to make you aware that the ESP is still a microcontrller and this is primarily a DTU. The Display code puts a heavy load on these devices and I haven't been a fan of this from day one.

If anything, I recommend using i2c displays. The bus inside the ESPs is not the fastest and pushing too much through one SPI is problematic.

@D3R-ST3FAN
Copy link

It's sad that there has been zero reaction since ages. I doubt that this is going to be implemented any time soon.
So far I have been using the fork @LennartF22 created. But this is not in sync with mainline since half a year.
Sounds like ahoy is the way to go then to have POE Shield support.

@markusdd
Copy link
Contributor

markusdd commented Jun 3, 2024

I don't know if it would be possible for @LennartF22 to at least rebase the fork to give people on PoE the latest greatest codebase, will be necessary anyway to go ahead with this.

But yeah, for mainline support ahoy is the only route right now, but it sucks for everybody who is relying on OpenDTU exclusive features. Tried to contact noby oon Discord as well but no respoonse yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants