-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
Implement dmx input with rdm support #64
Conversation
ba6f8f8
to
930506a
Compare
Sounds like great goals. Let me know if there are any specific questions you have while trying to complete this PR |
I am having trouble with a crash that happens when dmx is received while the wifi hotspot is being activated. When I wait until wifi has been activated before connecting dmx, it works fine. Edit: This can be circumvented by disabling the dmx driver inside 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. |
Calling 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? |
I have a specific question :D |
8c07246
to
046128f
Compare
@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. |
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. |
The dmx input side is more or less done and I would appreciate some reviews :-) 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: |
For readability reasons I would like to implement dmx output in a different PR. |
I had to put the dmx receiver into its own task on core 0. |
Sorry for not replying, having some health issues. Should hopefully be able to look at soon |
No need to apoligize :) Take your time! |
This is the first step in supporting both dmx input and dmx output on different pins.
This greatly improves readability because it gets rid of most of the ifdefs.
Some minor cleanup changes
No longer needed because missing ISR_ATTR have been added to esp_dmx.
This was necessary because otherwise it is not able to respond to rdm in time.
All required changes have been merged into esp_dmx upstream. Thus I removed my custom branch of esp_dmx. |
wled00/dmx_input.cpp
Outdated
return; | ||
} | ||
|
||
if (rxPin > 0 && enPin > 0 && txPin > 0) |
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.
Enable is actually optional with the newer RS485 adapters
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 esp_dmx library requires an en pin. The user can set any unused gpio if their rs485 adapter does not require an en pin. But some pin has to be set.
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.
Thought I was passing 0 or -1 before
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.
(when not required)
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 poked around a bit in esp_dmx and it looks like the pin has to fulfill GPIO_IS_VALID_OUTPUT_GPIO
. Otherwise esp_dmx will fail to initialize.
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 :) |
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 |
Yeah that's fine @softhack007 |
Are you able to resolve the conflicts @arneboe ? |
I can try to take a look on the weekend but no promises. |
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 |
Github let me resolve the conflict, but then doesn't give me permission to commit the result of "npm run build" @arneboe |
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 |
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 ❤️ )
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.