-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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 LAN8720 phy support (IDFGH-3260) #383
Conversation
Could you fix debug logging to use ESP_LOGD? |
The original code uses ESP_LOGI so I followed it by example. Should the original code remain ESP_LOGI and the new code use ESP_LOGD? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing this, we really appreciate it.
I've requested a couple of small changes/clarifications. The bigger questions around naming I'm happy for us to deal with later, unless you're particularly inspired to do some refactoring along those lines. :)
Regarding ESP_LOGD vs ESP_LOGI, as this is a fairly low-level technical example I think we can afford to keep the default ("info" level) output fairly technical like this. Some time down the road, if/when the PHY support code gets refactored into a library, we may change some of this.
#if (CONFIG_PHY_LAN8720) | ||
if (debug) | ||
dump_lan8720_registers(); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For code like the above it's preferable to define a single function name, something like static void dump_phy_registers
and name both PHY-specific versions the same. The #ifdef guards will determine which one is compiled in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original plan was to pull all of the phy code out of ethernet_main.c and put it in separate source files. That is a little more work but I'm happy to do that now if preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've refactored by moving the phy code out of ethernet_main.c so there is no phy device specific code there. Now would be the time to change the logging level before I check in. Preferences?
eth_config_t config; | ||
config.phy_addr = PHY31; | ||
// config.phy_addr = PHY31; | ||
config.phy_addr = PHY1; // tlk eval board |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an intentional change to the TLK1110 config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upstream repository uses PHY31 which would be specific to the original author and their hardware. I changed it to the default for the evaluation boards for both the TLK110 and LAN8720. I left the original commented out so the original author could easily change it. Ideally, what I should do is place the PHY ID in the menuconfig file.
config.phy_get_duplex_mode = phy_lan8720_get_duplex_mode; | ||
config.phy_get_partner_pause_enable = phy_lan8720_get_partner_pause_enable; | ||
config.phy_power_enable = phy_lan8720_power_enable; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the naming here either.
It may make more sense, long term, to split these PHY-specific functions into individual source files and have extern eth_config_t lan8270_eth_config
and extern eth_config_t tlk110_eth_config
or something like this.
You don't need to change that in this PR unless you want to, it's something we can rework in a follow-up (eventually some of this will probably be moved to library code, as well.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had planned to rework this if the code I submitted was of interest.
@@ -0,0 +1,17 @@ | |||
menu "Example Configuration" | |||
|
|||
config PHY_TLK110 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these kind of options, it's better to use a "choice". Something like this: https://github.com/espressif/esp-idf/blob/master/components/freertos/Kconfig#L14 (note the "choice" keyword starts the series of choices, until the "endchoice" line.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I looked at all of the Kconfig file examples under the examples tree and didn't see an example of that. I'll make the change.
|
||
static const char *TAG = "eth_demo"; | ||
extern void phy_device_check_phy_init(void); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to disagree with me here, but I think rather than declare all these functions as extern, it may be better to make them static in the individual source files and then declare inside each PHY-specific file
const eth_config_t tlk110_default_ethernet_phy_config = {
.phy_check_init = phy_tlk110_check_phy_init,
.phy_check_link = phy_tlk110_check_phy_link_status,
.phy_get_speed_mode = phy_tlk110_get_speed_mode,
.phy_get_duplex_mode = phy_tlk110_get_duplex_mode,
.flow_ctrl_enable = true;
.phy_get_partner_pause_enable = phy_tlk110_get_partner_pause_enable,
etc}
And then in this code:
extern const eth_config_t tlk110_default_ethernet_phy_config;
Which then be either passed to the esp_eth_init() directly (if the signature is changed to const), or copied to another structure which is then passed in directly.
This also has the advantage that you don't need to selectively compile the source files, we can compile both but only one will be linked into the final binary depending on which extern-ed config structure is used.
(Sorry if this seems different to what I was suggesting before, I realised I gave two different recommendations as I read my way through the code.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of functions in the eth_config structure that come from ethernet_main instead of the phy support code so a phy function like:
eth_config_t *eth_config = phy_device_get_config(eth_gpio_config_rmii, tcpip_adapter_eth_input);
or the structure would not be const and ethernet_main would just set those functions. Given that there may be more than one ethernet project in the future, the phy code should probably move out from the project and into a component support library (in the future). That would make me lean towards the eth_config function approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the latest suggestion. lan8720_phy.c has:
void phy_update_config(eth_config_t *config);
and ethernet_main.c has:
eth_config_t config;
config.gpio_config = eth_gpio_config_rmii;
config.tcpip_input = tcpip_adapter_eth_input;
config.phy_power_enable = phy_device_power_enable;
phy_update_config(&config);
ret = esp_eth_init(&config);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really a minor variation on your suggestion, but what about:
eth_config_t config = tlk110_default_ethernet_phy_config;
config.gpio_config = eth_gpio_config_rmii;
config.tcpip_input = tcpip_adapter_eth_input;
config.phy_power_enable = phy_device_power_enable;
ret = esp_eth_init(&config);
This means that you don't need to selectively compile each PHY source file, they can all be compiled but only one will be picked up at link time (because of the individual name tlk110_default_ethernet_phy_config
). This will probably help once the code is in a comonent support library (which I agree should be the goal), as many non-example users probably won't need to support multiple PHYs and therefore won't find the overhead of sdkconfig phy selection useful.
Eventually, we can probably also push some of these generic functions (like tcpip_adapter_eth_input) into the "default" config for each PHY as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked (in my version) that ethernet_main did not need to know anything about the underlying phy. Does it make a difference (other than compile time) whether you do selective compiling or selective linking? Your version would need ifdefs around the version of the struct to pull. If you feel strongly about it, I'll happily shut up and go with your version... :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of considerations:
- We try to use code-as-configuration where possible instead of sdkconfig. This often leads to more understandable code (if each PHY symbol has a different name, it's obvious what the intention of the phy configuration selection is from reading the calling code in ethernet_main. Whereas using the same name to configure PHYs means a reader has to also look at the component.mk file in order to see the full picture of what's happening, and how the three parts - sdkconfig, build configuration, ethernet_main - join together.)
- The second reason is that for Arduino and some other environments (like Micropython/NodeMCU) the maintainers build a single set of libraries and then link to those. For these environments to support multiple PHYs, they have to be able to access them all simultaneously without rebuilding or reconfiguring IDF.
Thanks for being patient with all my requests. :)
EDIT: Changed readable to understandable above, as you're totally correct that the version without #ifdefs reads clearer. It's just harder to understand exactly what it does, without looking at some additional pieces.
|
||
#include "lan8720_phy.h" | ||
|
||
extern char *demo_tag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to do something like static const char *TAG = "lan8720";
here, similar for the other PHY.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that works.
Thanks for the quick update, loosk good. I left a couple more comments. Feel free to let me know if you think it's better to go in a different direction, I'm not very familiar with the ethernet support yet (still waiting on a development board.) |
hello angus, best wishes |
Sorry, I used the wrong language. I mean a component (specifically a component which provides generic library-style support for PHYs, rather than keeping it in the example code.) We intend to keep the ethernet support fully open source. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your work on this, I like this structure a lot better overall.
@@ -32,92 +32,47 @@ | |||
#include "tcpip_adapter.h" | |||
#include "nvs_flash.h" | |||
#include "driver/gpio.h" | |||
#include "tlk110_phy.h" | |||
|
|||
extern void phy_update_config(eth_config_t *config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this declaration is a leftover here
} | ||
} | ||
// uncomment for console debug messages | ||
#define DEMO_DEBUG 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is not really "the IDF way". Use ESP_LOGD or ESP_LOGI to set the debug levels based on how important you think the output is.
Users can override the logging level (either per-file or globally) in various ways as described here:
http://esp-idf.readthedocs.io/en/latest/api/system/log.html
As I said before, I don't feel strongly about whether this example output should be ESP_LOGD or ESP_LOGI. Whichever you think is more useful is fine.
|
||
#define PHY_ID PHY0 | ||
// uncomment for console debug messages | ||
//#define PHY_DEBUG 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about debugging conventions applies here.
// select phy device | ||
//extern eth_config_t lan8720_default_ethernet_phy_config; | ||
//#define PHY_CONFIG lan8720_default_ethernet_phy_config | ||
extern eth_config_t tlk110_default_ethernet_phy_config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the example, it's nicer to conditionally compile this lines based on an sdkconfig choice (similar to the first version of this PR). Sorry if I accidentally gave you the impression that I wanted that removed.
Also, the "extern eth_config_t ..." declarations can be moved to the respective PHY headers.
phy_enable_flow_ctrl(); | ||
} | ||
|
||
eth_config_t lan8720_default_ethernet_phy_config = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be "const eth_config_t", in which case it lives in flash to save RAM.
phy_enable_flow_ctrl(); | ||
} | ||
|
||
eth_config_t tlk110_default_ethernet_phy_config = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be "const eth_config_t", in which case it lives in flash to save RAM.
Thanks Robin, looks very good. I've squashed these commits and put them into our internal review/merge queue. I'll let you know how things progress. We're still gearing up for the V2.0 release (which is already feature complete), so a final merge may be a little delayed. Thanks for being patient with us! |
Hi Robin, Quick update on this: no problems with the code surfaced in internal review. The only thing was some discussions around support-ability once merged to IDF. We're going to mark LAN8720 as contributed/unsupported when we merge, but we're also going to order some LAN8720 modules so we can test these changes in-house before merging and (hopefully) verify any other major ethernet changes that may impact LAN8270 (although we won't be guaranteeing support.) Thanks for being patient with us. Angus |
Thanks Angus. I've submitted a couple of new pull requests that move the phy support under the components tree and added ethernet support to the http_request example. These are obviously dependent on the original patch. Please take a look at the http_request pull request and let me know if that is the direction you would like to go. I hope to add ethernet support to all of the examples if that is ok. |
Hi @RobinCutshaw , Very nice board! Is the design open source anywhere? Sorry I didn't update you earlier. During our internal review process, we decided that one of our engineers should make a test setup with this PHY before we merge it. The engineer failed to get a LAN8270 breakout board working correctly, and then had higher priority work take him away from this. The task was handed back to me a couple of weeks ago. I have a LAN8720 board here but I haven't had time to wire it up yet. Hopefully soon. Thanks for being patient while we work through this. Angus |
perhabs this helps:
incredible everything is described neatly many manufacturer build ESP32 ETH PHY Boards olimex makes now ETH PHY + CAN too the most loved ones which makes the esp32 so interesting like LAN(ETH PHY) + CAN and the theme pSRAM .. @RobinCutshaw |
If you can point us to the description of correct wiring, it would probably speed up things significantly! |
WROVER-KIT Waveshare LAN8720 Breakoutmust_IO22 ->TX1 user_IO23
|
btw: the must ETH PHY RMII pins are from esp32 pin list RMII side 5 the details from the ESP32 technical reference manual edit:
same is then in the LAN8720 ;-) be sure you set phy addr right.
the EVB tlk110 used PHY0 the Waveshare LAN8720 breakboard Adapter used PHY1
|
Here are the pin definitions in the IDF code: `void eth_gpio_config_rmii(void)
} ` void phy_enable_flow_ctrl(void) Robin |
@sauttefk
thats one of more problems with the modules on market, but point in the wiki still to mounted things in the shematic .. best wishes |
@projectgus: could you please merge this pull request. I think it has been proven by several people here, that the code is working as intended. |
yes sure, but this means either
or
to get a 50MHz signal on the |
Thanks everyone for the information and for your patience with this. As I'm sure you can all appreciate, we have a lot of open tasks and sometimes things we'd love to see done are pushed behind more urgent items. I finally had time yesterday and I wired up my LAN8720, everything worked fine. I've also rebased this branch on the current master, and added a couple of things:
The branch is back in the internal review queue, so it may not be merged to master for a few days yet. I've pushed it temporarily/unofficially here: https://github.com/espressif/esp-idf/compare/temp/ethernet_lan8720 in case anyone would like to take a look or test it out beforehand. Expect a proper merge to master very shortly, though. Thanks again for bearing with us. PS @sauttefk Very nice blog post and trick using the spare pin on the Waveshare module for oscillator Enable! |
The ethernet_lan8720 branch works well on my ESP32 Etherbase board (which has the LAN8720). I like the other refactoring you did as well. |
ethernet: Add LAN8720 phy support, move PHY to components Encompasses PR #383 #383 Also includes changes to move PHY support functions into ethernet component, similar to #398 https://github.com/espressif/esp-idf/pull/398/files. See merge request !540
Cherry-picked in 5d6e6f7. Thanks for all the input folks, and thanks for sending the original PR @RobinCutshaw . |
Hi All, I see here that everybody is working with ESP32 and Ethernet. I have tried also with success the ESP32 and the LAN Board from wave share (DP83848) with small changes from the ESP32 example. Now I have a general question... waht about if I want to connect direct my PC to the ESP32 via the Ethernet interface, would that be possible? or at least has any sense? Thanks in any case for your contributions to the ESP32 community! Regards/Saludos/Gruesse Juan |
@santanapablo1975 , can you please share the DP83848 port ? i mean if you have made like a "phy_dp83848.h/c" sources ? Thanks ! |
For the Ethernet test I used this board
http://www.waveshare.com/dp83848-ethernet-board.htm
And took the example from the ESP32 IDF with small change in the PHYADR1
and create the pinout for pin CRS
Please let me know how your test was doing!
Regards!!!
Juan
…On Mon, May 1, 2017 at 11:18 AM, ahmedjouirou ***@***.***> wrote:
@santanapablo1975 <https://github.com/santanapablo1975> , can you please
share the DP83848 port ? i mean if you have made like a "phy_dp83848.h/c"
sources ?
Thanks !
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#383 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AT6ZYJww-ImIkJuM_ADBnFIsV9fNCI3nks5r1gXOgaJpZM4MMeJj>
.
|
@ESP32DE Where should we connect PIN_PHY_POWER on lan8720 board? |
Hey @projectgus rst:0x1 (POWERON_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT) rst:0x10 (RTCWDT_RTC_RESET),boot:0x3 (DOWNLOAD_BOOT(UART0/UART1/SDIO_REI_REO_V2)) rst:0x1 (POWERON_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT) //After pressing reset for the 3rd time I got in to the right mode TIA |
The PIN_PHY_POWER is normally used to turn on/off the power to the ethernet PHY or to control a mosfet or clock enable pin (that you add in your design) that blocks the 50MHz clock from GPIO0. If the clock is running and connected to GPIO0 when you boot, you have a 50/50 chance of going into bootloader mode or normal boot. If you are using an external PHY board to test, you will need to manually remove power or disconnect the clock from GPIO0 while booting. |
So if I make those changes as suggested by @sauttefk connecting the PIN_PHY_POWER pin to the clock enable of lan8720 would that work without having to continuously reset to get in to the right mode. |
I think you need a custom circuit that would disable clock via the clock
enable pin on the oscillator (if you are using one) or would need to block
the clock from GPIO0 through something like a mosfet.
…On Thu, Jul 20, 2017 at 8:57 AM, Nakul93 ***@***.***> wrote:
So if I make those changes as suggested by @sauttefk
<https://github.com/sauttefk> connecting the PIN_PHY_POWER pin to the
clock enable of lan8720 would that work without having to continuously
reset to get in to the right mode.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#383 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABntlL484fy8B6oe9SPsEseF2qxEXHrZks5sP064gaJpZM4MMeJj>
.
|
Hello, |
@Arthedian Take a look here: https://github.com/espressif/esp-idf/blob/09d2791cfdf7348405712c0a746ab780b2b0230d/examples/ethernet/ethernet/README.md#phy-clock-wiring |
@Arthedian You may be able to reconfigure to generate the clock from the ESP32 and output on GPIO 16, as noted at the link @sauttefk sent. You can set the clock mode in the eth_config_t structure passed when you configure the clock, see here: |
Thanks for your support. Fortunately now I got ESP32 with PIN0, so its no problem. But when I connected it to the LAN8720 (I double check the connection), it gives me this error. Do you know, where the problem could be? E (1304) emac: Timed out waiting for PHY register 0x2 to have value 0x0007 (mask 0xffff). Current value 0xffff EDIT: Fixed, when |
Hello, I have another question. Can i have ethernet connected to esp and load the code at once? I already have transistor between pin0 and ethernet. |
Hi Juan, i am trying to interface DP83848 with ESP32, i am facing some issue, i would like to know what all possible changed needed in phy_xxxx.c file for this PHY. would like to know your inputs. Regards |
Sorry for pulling this old thread. Is there any way to get the lan8720ai working with a esp 32 Pico d4? As far as I can see, The gpio 16 and 17 are used for the internal flash. I already asked here, but no answer so far. Thanks in advance. |
Dear,
This is my project about *Esp32-Wroom32u* and LAN8720.
[image: image.png]
Compare to *ESP32-Pico D4*
[image: image.png]
You can see that function of pins for Ethernet comunication is the similar
and doesn't conflict.
Best Regards,
Nguyen Vi Tan Tu (Mr.)
Electrical Engineer - Hardware Developer
Mobile: (+84) 78 212 0323
Vào Th 6, 8 thg 5, 2020 vào lúc 16:01 BenjaminK87 <
notifications@github.com> đã viết:
… Sorry for pulling this old thread.
Is there any way to get the lan8720ai working with a esp 32 Pico d4? As
far as I can see, The gpio 16 and 17 are used for the internal flash.
I already asked here, but no answer so far.
https://www.esp32.com/viewtopic.php?f=12&t=15496
Thanks in advance.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#383 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADLFTQYDQWHP7GXW6AWEKNLRQPC6XANCNFSM4DBR4JRQ>
.
|
Thank your for your reply. I think something went wrong with the picture upload. I can't see it. |
No description provided.