MoonModules / WLED

Control WS2812B and many more types of digital RGB LEDs with an ESP8266 or ESP32 over WiFi! MoonModules adds features on top of upstream.
https://mm.kno.wled.ge
European Union Public License 1.2
228 stars 70 forks source link

Implement dmx input with rdm support #64

Closed arneboe closed 9 months ago

arneboe commented 1 year ago

Currently I am maintaining an old fork of WLED that supports dmx input with rdm using the esp_dmx library. I would like to get rid of that fork and instead use MoonModules with as little changes as possible. My usecase is having dmx input with rdm and dmx output work at the same time (using two max495 on port 1 and 2).

I worked through @netmindz dmx input and dmx output branches trying to get them to work and adapt them to my usecase.

Juggling those branches was kinda challenging for me because they are tracking and old commit of WLED and an old version of esp_dmx and dmx input and output where kinda entangled. Thus I decided to clean it up a bit. That kinda got out of hand and here we are. (I hope you don't mind :heart: )

The goals for this PR are:

edit: From my point of view this PR is ready to merge. It is working as intended in my test cases.

netmindz commented 1 year ago

Sounds like great goals. Let me know if there are any specific questions you have while trying to complete this PR

arneboe commented 1 year ago

I am having trouble with a crash that happens when dmx is received while the wifi hotspot is being activated. I am getting the "Cache disabled but cached memory region accessed" panic. My guess is, that activating wifi disables the cache. But the dmx driver has been placed in IRAM so this shouldn't happen?

When I wait until wifi has been activated before connecting dmx, it works fine. But this is not a feasible solution because devices are connected to the dmx bus before they are turned on :D

Edit: This can be circumvented by disabling the dmx driver inside WLED::initConnection().

Edit: This also happens when accessing the web interface. I have no idea where the entrypoint for the webinterface code is. I.e. a function that is always called before the cache is disabled.

arneboe commented 1 year ago

Calling dmx_receive with zero timeout does never receive any data. I am unsure if this is a bug or intended behavior. I created an issue https://github.com/someweisguy/esp_dmx/issues/87 on esp_dmx to resolve that question.

If this is intended behavior, I will probably have to move the dmx_receive code over into its own task. Do you see any problems with that? And should I pin it to core 0 or 1? Afaik all the arduino stuff runs on core 1, so maybe pinning it to core 0 is a good idea?

arneboe commented 1 year ago

Sounds like great goals. Let me know if there are any specific questions you have while trying to complete this PR

I have a specific question :D If the user changes the dmx start address using rdm, how can I store it permanently? Probably I have to use the same code path that the web interface is using. But i cannot figure out how the web interface saves data. edit: found it ;)

softhack007 commented 1 year ago

arneboe force-pushed the dmx_input branch

@arneboe please try to avoid force-pushing. It may cause lots of weird behaviour on github later.

If you need to copy single commits from one branch to another, it's better to use cherry-picking. It even works between different repositories.

https://docs.github.com/en/desktop/contributing-and-collaborating-using-github-desktop/managing-commits/cherry-picking-a-commit-in-github-desktop

arneboe commented 1 year ago

arneboe force-pushed the dmx_input branch

@arneboe please try to avoid force-pushing. It may cause lots of weird behaviour on github later.

If you need to copy single commits from one branch to another, it's better to use cherry-picking. It even works between different repositories.

https://docs.github.com/en/desktop/contributing-and-collaborating-using-github-desktop/managing-commits/cherry-picking-a-commit-in-github-desktop

I am force pushing for several reasons:

Both of those shouldn't cause problems when merging later. Rebasing makes the merge easier and modifying my own commits shouldn't matter because they do not exist on the main branch.

I agree, that copying single commits from somewhere should be done by cherry picking. But that is not what I am doing here.

arneboe commented 1 year ago

The dmx input side is more or less done and I would appreciate some reviews :-) It is tested and working using a Botex RDM/DMX tester. But that is just one test. Maybe someone else could test it aswell.

However, it relies on changes that I made to esp_dmx that are stil under review. Thus for now this PR is tracking my fork of esp_dmx until the following PRs are merged: https://github.com/someweisguy/esp_dmx/pull/90 https://github.com/someweisguy/esp_dmx/pull/92 https://github.com/someweisguy/esp_dmx/pull/93

arneboe commented 1 year ago

For readability reasons I would like to implement dmx output in a different PR. Otherwise this gets really hard to review. Also, I am not sure if there is any benefit in using esp_dmx for the dmx output if the sparkfun lib works just fine? What do you think? @netmindz @softhack007

arneboe commented 1 year ago

I had to put the dmx receiver into its own task on core 0. Otherwise it was not able to respond to rdm requests in time (there is only a few micros to respond due to how rdm works). I am not sure if this breaks anything else?.It works for me :D

netmindz commented 1 year ago

Sorry for not replying, having some health issues. Should hopefully be able to look at soon

arneboe commented 1 year ago

Sorry for not replying, having some health issues. Should hopefully be able to look at soon

No need to apoligize :) Take your time!

arneboe commented 1 year ago

All required changes have been merged into esp_dmx upstream. Thus I removed my custom branch of esp_dmx. I rebased this branch on top of mdev again to keep it easy to merge.

netmindz commented 1 year ago

Sorry it's taken so long to get back to you @arneboe

Overall I would say great contribution, you have cleaned up my initial implementation quite a bit, just a few minor issues to address

arneboe commented 1 year ago

Sorry it's taken so long to get back to you @arneboe

Overall I would say great contribution, you have cleaned up my initial implementation quite a bit, just a few minor issues to address

Thanks for the review. I ll fix everything within the next days :)

netmindz commented 11 months ago

Sorry, I've not tested this yet @arneboe I've not got a suitable test setup at the moment. I have shared links to this on Discord and asked others to help test but I've not heard back. Can you try and get others to test too? You will also need to resolve the merge conflicts before I can merge

softhack007 commented 11 months ago

Hi guys @arneboe @netmindz, I'm preparing for a MM release this weekend. Would it be ok for you if we merge DMX changes after the release?

netmindz commented 11 months ago

Yeah that's fine @softhack007

netmindz commented 10 months ago

Are you able to resolve the conflicts @arneboe ?

arneboe commented 10 months ago

I can try to take a look on the weekend but no promises.

netmindz commented 10 months ago

Thanks. It's really just the platformio change that matters, for the html stuff it will just be running the npm task and submitting your change

netmindz commented 10 months ago

Github let me resolve the conflict, but then doesn't give me permission to commit the result of "npm run build" @arneboe

netmindz commented 10 months ago

I've made a few tweaks, if you can merge the the MM dmx_input branch into yours, then I think we are good to then accept this PR @arneboe