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

Added ETH support for the cod.m WLED LAN Controller #4160

Closed
wants to merge 1 commit into from

Conversation

UtechtDustin
Copy link

Added ethernet configuration for the upcoming cod.m WLED LAN Controller.

@blazoncek
Copy link
Collaborator

There are no differences to the existing ESP32-POE Ethernet type. Why a new one?

  // ESP32-POE
  {
     0,                   // eth_address,
    12,                   // eth_power,
    23,                   // eth_mdc,
    18,                   // eth_mdio,
    ETH_PHY_LAN8720,      // eth_type,
    ETH_CLOCK_GPIO17_OUT  // eth_clk_mode
  },

@UtechtDustin
Copy link
Author

UtechtDustin commented Sep 25, 2024

If a customer of us will reset the controller it's easier to find the correct config.
Same thing like QuinLed-ESP32-Ethernet <-> TwilightLord-ESP32 Ethernet Shield.

@softhack007
Copy link
Collaborator

If a customer of us will reset the controller it's easier to find the correct config. Same thing like QuinLed-ESP32-Ethernet <-> TwilightLord-ESP32 Ethernet Shield.

Instead of adding a duplicate config, it's better to update the existing UI dropdown entry.
--> "ESP32-POE Ethernet or cod.m controler".

Case solved 😉

@softhack007 softhack007 added the cosmetic change that shall not have functional impact (whitespace trimming, typo correction, debug messages) label Sep 25, 2024
@blazoncek
Copy link
Collaborator

Same thing like QuinLed-ESP32-Ethernet <-> TwilightLord-ESP32 Ethernet Shield

These were added at the time when space was not yet at premium (more than 3 years ago). Unfortunately removing them is a breaking change that may render devices unreachable.

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

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

Adding a new type is only justified if it differs in HW configuration.

@UtechtDustin
Copy link
Author

UtechtDustin commented Sep 26, 2024

@blazoncek can we rename the option as @softhack007 suggested ?
That was also our first idea, but that could end up in something like that ESP32-POE Ethernet or cod.m controller or xyz controller or foobar ETH controller.

@blazoncek
Copy link
Collaborator

I'm afraid this will lead nowhere.
It will inevitably end in "Please add Adapter XXX to the list". Unfortunately I do not have an answer how should we address this issue but adding "yet another name" to the selection list is not the correct approach long term.

IMO provide a user manual where it is clearly stated that your board uses "ESP32-POE" ethernet adapter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cosmetic change that shall not have functional impact (whitespace trimming, typo correction, debug messages)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants