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

Create ArtnetEtherENC.h #79

Merged
merged 22 commits into from
Jan 10, 2024
Merged

Create ArtnetEtherENC.h #79

merged 22 commits into from
Jan 10, 2024

Conversation

MrFrangipane
Copy link
Contributor

Hello

Thank you for this awesome library.

Is it possible to make possible the use of the following library ?

Allows to use https://github.com/JAndrassy/EthernetENC

For now, I modifiy files manually, it would be awesome to have this embedded in the lib, if It helps someone else

All the best,
Val

Copy link
Owner

@hideakitai hideakitai left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Could you add examples and workflows for them?

@MrFrangipane
Copy link
Contributor Author

Hi !

Thanks for your response !

I'll look into it :)

Val

@MrFrangipane
Copy link
Contributor Author

MrFrangipane commented Jan 8, 2024

Hi,

I tried to add what you requested :)

The wiring diagram was taken from https://github.com/Juddling/pi-pico-enc28j60 and is reportedly made by https://github.com/tobiasvogel

I'm not familiar with workflows I copied/pasted previous sections

Hope this helps,
All the best !

@MrFrangipane
Copy link
Contributor Author

Also,

It's probably not the place to ask you about this (sorry), I see here than only four callbacks are allowed https://github.com/hideakitai/ArtNet#subscribing-callbacks-with-net-sub-net-and-universe-as-you-like, why is it so ?

Thank you again,
Val

@hideakitai
Copy link
Owner

About callbacks, the limitation has been removed, but README is not fixed…

#59

Copy link
Owner

@hideakitai hideakitai left a comment

Choose a reason for hiding this comment

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

Commented

Comment on lines 249 to 256
board:
- vendor: rp2040
arch: rp2040
name: rpipico
include:
- index: https://github.com/earlephilhower/arduino-pico/releases/download/global/package_rp2040_index.json
board:
vendor: rp2040
Copy link
Owner

Choose a reason for hiding this comment

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

I think EtherENC is available for any platform. If so, please add “board” and “include” as same as the “build-ethernet”.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, will do

README.md Outdated
@@ -41,6 +41,20 @@ If you have already installed this library, please follow:
- ESP32 (Ethernet and ETH)
- ESP8266
- Almost all platforms without WiFi
- Raspberry Pi Pico with ENC28J60 (please read following section)
Copy link
Owner

Choose a reason for hiding this comment

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

All platforms that can use EtherENC can be supported, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right,

I'll check the library documentation

@MrFrangipane
Copy link
Contributor Author

About callbacks, the limitation has been removed, but README is not fixed…

#59

Oh OK i was surprised to see it working, do you want me to remove this section from the readme as well ?

@hideakitai
Copy link
Owner

Oh OK i was surprised to see it working, do you want me to remove this section from the readme as well ?

It’s great if you can! Thank you!

@MrFrangipane
Copy link
Contributor Author

I see some builds have failed

Should we remove those boards from the build and list supported boards in the docs ?

  • UNO has not enough memory

  • I see errors regarding MAC Address (which I had to remove)

   /home/runner/Arduino/libraries/ArtNet/Artnet/ArtPollReply.h:68:15: warning: 'my_mac' is used uninitialized in this function [-Wuninitialized]
           memcpy(r.mac, my_mac, 6);
           ~~~~~~^~~~~~~~~~~~~~~~~~

@MrFrangipane
Copy link
Contributor Author

MrFrangipane commented Jan 8, 2024

Oh OK i was surprised to see it working, do you want me to remove this section from the readme as well ?

It’s great if you can! Thank you!

It's done, I copied the explanation you gave in #59

https://github.com/hideakitai/ArtNet/blob/b4df2244faa6df0d9837bcf8c3ecfa981039d0bf/README.md#subscribing-callbacks-with-net-sub-net-and-universe-as-you-like

Although I'm not sure what is meant by "making public 4 of them"

@hideakitai
Copy link
Owner

Although I'm not sure what is meant by "making public 4 of them"

PollReply has the information of four input/output ports. So current implementation is, you can subscribe many universes but four of them are informed by PollReply. (But this implementation is not preferred, so I should fix it https://art-net.org.uk/how-it-works/discovery-packets/artpollreply/)

Copy link
Owner

@hideakitai hideakitai left a comment

Choose a reason for hiding this comment

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

Added suggestion for README

README.md Outdated
- These universes (targets of the callbacks) are reflected to `net_sw` `sub_sw` `sw_in` in `ArtPollreply` automatically

PortTypes, GoodInput/Output, SwIn, etc., are limited to 4 ports. If more than 4 callbacks are needed, it is better to subscribe to more than four and make public 4 of them
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
PortTypes, GoodInput/Output, SwIn, etc., are limited to 4 ports. If more than 4 callbacks are needed, it is better to subscribe to more than four and make public 4 of them
PortTypes, GoodInput/Output, SwIn, etc., are limited to 4 ports. Only the first four ports are reflected if you subscribe to more than four callbacks.

Comment on lines 306 to 307
sketch-paths: |
- examples/EthernetENC
Copy link
Owner

@hideakitai hideakitai Jan 9, 2024

Choose a reason for hiding this comment

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

Following sketch-paths is better to avoid memory shortage for avr

Suggested change
sketch-paths: |
- examples/EthernetENC
sketch-paths: |
- examples/EthernetENC/receiver
- examples/EthernetENC/sender

Comment on lines 250 to 252
- vendor: arduino
arch: avr
name: uno
Copy link
Owner

@hideakitai hideakitai Jan 9, 2024

Choose a reason for hiding this comment

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

Could you comment out these lines and add a comment like "temporarily disabled because of memory shortage of sender example"

https://github.com/hideakitai/ArtNet/actions/runs/7453947055/job/20282884214?pr=79

Comment on lines 322 to 324
#if !defined(ESP8266) && !defined(ARTNET_ETHER_IS_ENC28J60)
Ethernet.MACAddress(mac);
#endif
Copy link
Owner

Choose a reason for hiding this comment

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

The latest (on GitHub) version has the MACAddress() method.

https://github.com/JAndrassy/EthernetENC/blob/3a50d3aa9c1510e4090d1763ec3e91c700b764a1/src/Ethernet.h#L99

So, could you modify these lines as follows? (ESP8266 also has MACAddress() method now, #if directive has removed)

Suggested change
#if !defined(ESP8266) && !defined(ARTNET_ETHER_IS_ENC28J60)
Ethernet.MACAddress(mac);
#endif
Ethernet.MACAddress(mac);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,

I was able to compile my sketch after cloning the latest ENC library and removing the #if directive

Readme and workflow have been updated to use git instead of regular installation

@hideakitai
Copy link
Owner

@MrFrangipane Maybe the suggestions above can fix workflows. Could you try to implement them?

@MrFrangipane
Copy link
Contributor Author

Although I'm not sure what is meant by "making public 4 of them"

PollReply has the information of four input/output ports. So current implementation is, you can subscribe many universes but four of them are informed by PollReply. (But this implementation is not preferred, so I should fix it https://art-net.org.uk/how-it-works/discovery-packets/artpollreply/)

Thank you ! I get it now :)

@MrFrangipane
Copy link
Contributor Author

@MrFrangipane Maybe the suggestions above can fix workflows. Could you try to implement them?

Sure, changes have been made :)

I realized too late i could directly commit your suggestions, I'll know for next time

@hideakitai
Copy link
Owner

Ah, maybe this is a bug of EthernetENC…

@MrFrangipane
Copy link
Contributor Author

Ah, maybe this is a bug of EthernetENC…

It says on EthernetENC's readme

For ESP32 it is better than using a MAC layer module like the enc28j60 to use a PHY layer LAN module like LAN8720 supported by the ESP32 MAC peripheral and the Arduino boards support package.

Should we drop ESP32 from those tests and state to not use ESP32 with EthernetENC in the readme ?

@hideakitai
Copy link
Owner

Please comment out esp32, s3, and c3 from workflow. But it is NOT unavailable for esp32 series, I will send pull request to EtherENC :)

@hideakitai
Copy link
Owner

state to not use ESP32 with EthernetENC in the readme ?

Could you add a same note as README of EtherENC?

For ESP32 it is better than using a MAC layer module like the enc28j60 to use a PHY layer LAN module like LAN8720 supported by the ESP32 MAC peripheral and the Arduino boards support package.

@hideakitai
Copy link
Owner

hideakitai commented Jan 10, 2024

Created a PR on EthernetENC Networking-for-Arduino/EthernetENC#58

-> merged

@hideakitai
Copy link
Owner

If you finish working on this PR, please mention me!

@MrFrangipane
Copy link
Contributor Author

MrFrangipane commented Jan 10, 2024

Hi @hideakitai

Thank you for your help again !

I realized only 2 examples were compiled, so I added them to the workflow

sketch-paths: |
  - examples/EthernetENC/receive_fastled
  - examples/EthernetENC/receiver
  - examples/EthernetENC/send_receive
  - examples/EthernetENC/sender

This revealed that FastLED is not supporting mkrvidor4000 boards. What do you prefer ?

  • remove the board from tests and leave fastled enabled for the others
  • remove the FastLED tests for all boards and leave mkrvidor4000 board enabled

I checked the other workflows and they seem to correspond to the 2nd option. The current version of the workflow is as follows

sketch-paths: |
  - examples/EthernetENC/receiver
  - examples/EthernetENC/send_receive
  - examples/EthernetENC/sender
# disabled due to FastLED not supporting mkrvidor4000
# - examples/EthernetENC/receive_fastled

If this is OK for you, merging should be OK 🥳

@hideakitai
Copy link
Owner

hideakitai commented Jan 10, 2024

Thank you! Yeah, mkrvidor is not supported. Hmm.. I prefer

remove the board from tests and leave fastled enabled for the others

@MrFrangipane
Copy link
Contributor Author

Thank you! Yeah, mkrvidor is not supported. Hmm.. I prefer

remove the board from tests and leave fastled enabled for the others

@hideakitai board has been disabled and FastLED reinstated

All tests seems to have passed :)

@hideakitai
Copy link
Owner

All checks passed, thank you! I'll review the code again :)

Copy link
Owner

@hideakitai hideakitai left a comment

Choose a reason for hiding this comment

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

There is one typo, but LGTM

README.md Outdated Show resolved Hide resolved
Co-authored-by: Hideaki Tai <hideaki.tai@gmail.com>
@MrFrangipane
Copy link
Contributor Author

There is one typo, but LGTM

Suggested fix was commited :)

@hideakitai hideakitai merged commit 41ee725 into hideakitai:main Jan 10, 2024
32 checks passed
@hideakitai
Copy link
Owner

Thank you for your contribution!

@hideakitai
Copy link
Owner

Please close #80 :) Thanks again!

@MrFrangipane
Copy link
Contributor Author

Awesome !

Thank you for all those wonderful libraries 🙏

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.

2 participants