Usbdmxcom #1766
Usbdmxcom #1766
Conversation
Still needs some tidying and actual DMX generation protocol code adding
plugins/usbdmx/AsyncPluginImpl.cpp
Outdated
| bool AsyncPluginImpl::NewWidget(USBDMXCom *widget) { | ||
| return StartAndRegisterDevice( | ||
| widget, | ||
| // TODO(Someone): Add the serial here like ShowJockey if present) |
There was a problem hiding this comment.
Does it have a serial number? Do you need a hand with this?
There was a problem hiding this comment.
The serial can change from device to device. See https://www.usbdmx.com/building_8.html
If it is for device identification, we can do a case insensitive search for "usbdmx.com" in the manufacturer field?
There was a problem hiding this comment.
It's just about showing the serial number in the description within OLA. Take a look at the ShowJockey code, it's pretty trivial.
There was a problem hiding this comment.
I added the widget->SerialNumber(), but I obtain only "USBDMX.com Device (4-20)" .. why "4-20" ?
plugins/usbdmx/SyncPluginImpl.cpp
Outdated
| bool SyncPluginImpl::NewWidget(USBDMXCom *widget) { | ||
| return StartAndRegisterDevice( | ||
| widget, | ||
| // TODO(Someone): Add serial if present like ShowJockey |
| * Create a USBDMX.com message to match the supplied DmxBuffer. | ||
| */ | ||
| size_t CreateFrame(const DmxBuffer &buffer, uint8_t frame[USBDMXCOM_MAX_FRAME_SIZE]) { | ||
| static uint8_t old_Values[512]; |
There was a problem hiding this comment.
Have a look at #1746 , you can just copy the buffer rather than writing all the values into another array just for comparison purposes.
There was a problem hiding this comment.
Where do i need to look at?
I made this this way, because no need to make a full copy if only one value changed, and i still have to compare them all..
There was a problem hiding this comment.
See OpenDeckWidget::SendDMX and it's internal_buffer:
https://github.com/OpenLightingProject/ola/pull/1746/files#diff-6ef0f8bbea30618491c7d8a882f9f77665c1c915218980ef16d13ffd59022c2bR46
Where do i need to look at? I made this this way, because no need to make a full copy if only one value changed
OLA does some magic called copy on write so it sometimes won't even make a copy!
i still have to compare them all..
You could do an equality check of the buffer, which does a nice efficient memcmp to decide if there is even anything to update.
Indeed see IsBufferDiff in the other PR...
There was a problem hiding this comment.
As you already known your architecture, why not accepting the PR and then modify everything you want, then let me see if it still works?
There was a problem hiding this comment.
I don't really want to merge a PR which doesn't work or potentially doesn't, but I can try and help you with some of the changes within this PR.
But also a bit of this:
https://www.openlighting.org/ola/get-help/ola-faq/#Can_you_add_support_for_the_ltinsert-name-heregt_device
plugins/usbdmx/USBDMXCom.cpp
Outdated
| frame[frame_length++] = 0x26; // No-Op | ||
| frame[frame_length++] = 0x44; // DMX TX ON |
There was a problem hiding this comment.
Can we have all these magic numbers as constants please!
There was a problem hiding this comment.
Some initial comments.
Let me know if you want assistance on the todo items, and particularly for DMX input whether you want to merge in the current state and add DMX input in a separate PR or get it all done in one?
Some initial comments.
Let me know if you want assistance on the todo items, and particularly for DMX input
Oh yes please, assistance is much appreciated, as OLA is complex for me, and also, i'm new to the PR workflow!
whether you want to merge in the current state and add DMX input in a separate PR or get it all done in one?
Maybe little steps by little steps?
There was a problem hiding this comment.
Can we have all these magic numbers as constants please!
At least this one should be an easy fix :)
plugins/usbdmx/USBDMXCom.cpp
Outdated
| if (value != old_Values[channel]) { | ||
| old_Values[channel] = value; | ||
| OLA_DEBUG << "Ch. " << channel << " = " << (int)value; | ||
| frame[frame_length++] = (channel < 256) ? 0x48 : 0x49; |
There was a problem hiding this comment.
Again for the magic numbers.
| uint8_t frame[USBDMXCOM_MAX_FRAME_SIZE]; | ||
| size_t frame_length = CreateFrame(buffer, frame); | ||
| if (frame_length == 0) { | ||
| return true; |
There was a problem hiding this comment.
Probably chuck an OLA_INFO or DEBUG line in here so we know it deliberately hasn't sent anything.
There was a problem hiding this comment.
That would flood the console output even when nothing change, but you're the boss here
There was a problem hiding this comment.
That's why it should probably only be a DEBUG.
But at least while we're starting out it's worth having. Often people will report an issue and if you're lucky, generally all you ever get out to work on is an OLA_DEBUG level log, so the more detail the better as far as I'm concerned.
There was a problem hiding this comment.
Ok, but there is already useful OLA_DEBUG << "Ch. " << channel << " = " << static_cast<int>(value); which will be made unwatchable by the flood. It is okay to replace it by OLA_INFO so ?
Co-authored-by: Peter Newman <peternewman@users.noreply.github.com>
|
There are still some outstanding lint issues: I've resynced with master to make some unrelated changes vanish, so you'll need to pull on your PC before recompiling. |
Okay, i did git pull, then git merge master. and now i'm recompiling ... then i'll try to fix some issues... |
Co-authored-by: Peter Newman <peternewman@users.noreply.github.com>
Co-authored-by: Peter Newman <peternewman@users.noreply.github.com>
|
Hmm. I've updated the branch. Can we work this someday? i'm feeling all that work and efforts will be wasted x.X |
This code is working on the hardware. Still things needs to be done: