-
Notifications
You must be signed in to change notification settings - Fork 204
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
Usbdmxcom #1766
base: master
Are you sure you want to change the base?
Usbdmxcom #1766
Conversation
Still needs some tidying and actual DMX generation protocol code adding
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.
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?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it have a serial number? Do you need a hand with this?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below:
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 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 |
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.
Again
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.
done
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have all these magic numbers as constants please!
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would flood the console output even when nothing change, but you're the boss here
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
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 <[email protected]>
Co-authored-by: Peter Newman <[email protected]>
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: