Add support for Arctic RGB Controller #26
Conversation
| #define ARCTIC_COMMAND_COMMAND_OFFSET (sizeof(header)) | ||
| #define ARCTIC_COMMAND_PAYLOAD_OFFSET (sizeof(header) + 1) | ||
| | ||
| const unsigned char header[] = { |
There was a problem hiding this comment.
Put opening braces on their own lines unless the entire data is a single line. Fix all occurrences.
| { | ||
| RGBController_Arctic *rgb_controller = new RGBController_Arctic(controller); | ||
| ResourceManager::get()->RegisterRGBController(rgb_controller); | ||
| } else { |
There was a problem hiding this comment.
Braces and else on own lines
| : controller(arctic_controller), | ||
| keepalive_thread_run(true) | ||
| { | ||
| name = "Arctic RGB Controller"; |
There was a problem hiding this comment.
Line up =. Apply to all blocks of assigment operators.
RGBController/RGBController.cpp Outdated
| | ||
| void RGBController::RegisterUpdateCallback(RGBControllerCallback new_callback, void * new_callback_arg) | ||
| { | ||
| UpdateMutex.lock(); |
There was a problem hiding this comment.
Don't touch common code in device MRs. This needs to be its own MR with a detailed explanation of why you're changing the core RGBController API code.
| Added comments. Looks good, just some style stuff except for touching RGBController.cpp. Any changes to core code I want to thoroughly review, submit as separate MR with detailed reason for change. |
Disabling output processing is necessary since otherwise sending a LF character (ASCII 0x0A) will automatically insert a CR character (ASCII 0x0D). Disabling input processing should prevent this when receiving data.
| Ok, i will fix the remaining issues. |
Wer-Wolf force-pushed the arctic-controller branch from 6b9fefe to ae225c4 Compare The Arctic RGB controller support 4 RGB channel and can be controlled over a CH341 USB-to-serial chip. The controller support two commands, one for identifying the controller on a serial port and one for setting the RGB values for each RGB channel. Since the controllers disables the RGB channels after ~1s, a keepalive thread is used.
Wer-Wolf force-pushed the arctic-controller branch from ae225c4 to 5856590 Compare | The implication is that every zone have at least 1 LED, at least unless it is an ARGB header zone in which case it can have 0 LEDs but a nonzero max LED count which allows the user to resize between 0 and max LEDs in the zone. A 0 LED zone that is not resizable makes no sense. What would be the reason for a 0 LED zone? |
| I thought about a zone with an unknown number of LEDs, but if those dummy LEDs are necessary, then its fine. |
| Is this a 12V analog RGB controller? An LED in OpenRGB terms is the smallest unit of RGB control, so a single color RGB strip is considered one LED even though it may have hundreds of actual LED chips. They can only be controlled as a single unit thus a single LED. |
| { | ||
| UpdateLEDs(); // Already protected thru a device update thread | ||
| std::this_thread::sleep_for(ARCTIC_KEEPALIVE_PERIOD); | ||
| } else { |
There was a problem hiding this comment.
Braces and else on own lines
| \*-------------------------------------------------------------------*/ | ||
| | ||
| RGBController_Arctic::RGBController_Arctic(ArcticController *arctic_controller) | ||
| : controller(arctic_controller), |
There was a problem hiding this comment.
Follow the convention of other controllers. The arctic_controller pointer should be renamed to controller_ptr and the initialization should be inside the function:
RGBController_Arctic::RGBController_Arctic(ArcticController* controller_ptr)
{
controller = controller_ptr;
Also make sure it's named controller_ptr in the .h file
| Some more comments I didn't spot earlier since I was on my phone. Just formatting stuff. The LED and zone initialization looks good to me. |
| Manually merged |
| Looks like this is problematic. Trying to do autodetection with find_usb_serial_port on all CH341 devices is a very bad idea. We typically do use find_usb_serial_port to find the USB serial path associated with a given device, but the problem is CH341 is not a unique ID to the Arctic RGB controller, it's a chip that is extremely widely used across tons of different devices. Most notably, it is widely used on clone Arduino boards and people often use these Arduino boards to run RGB controllers (Adalight, TPM2, Keyboard Visualizer, etc). I'm debugging an issue where Keyboard Visualizer Arduino controllers aren't working with the current development releases of OpenRGB 0.9 before I release 1.0 and I discovered that the issue is being caused by ArcticController hijacking the already-opened /dev/USBx serial port and changing the baud rate from 115200 to 250000 and doing its autodetection, failing, and leaving the port in this broken state. Then, in LEDController, when it tries to send the Keyboard Visualizer protocol (which needs to be 115200 baud) it doesn't realize the port is at 250000 baud and sends garbage to the Arduino. If there is no way to determine that this is an Arctic controller based on the USB information alone, I don't think we should be poking all CH341 ports to autodetect it. This should be manual, or at very least manually enabled in the settings. Either that, or we need an absolutely reliable method of reading the previous port settings in their entirety, doing the probe, and then setting them back. |
| Does the Arduino Keyboard visualizer also probe automatically? |
| I think it would be OK if users have to manually instantiate the Arctic RGB controller. However i have no clue how to implement this. Any suggestions? |
| I ended up finding a workaround. It looks like Windows locks the COM port as soon as you open it so that it can't be opened a second time, but on Linux it does allow two file descriptors to point to the same tty device, which means the second open() doesn't fail. I added a lock using flock() whenever the first thing opens the port, which prevents the Arctic controller from being able to open the port a second time. I think this is acceptable for avoiding conflicts with other RGB devices, though it could still affect things outside of OpenRGB. |
| I think we should still require users to manually instantiate the Arctic controller, just in case. But using flock() under Linux seems like a good thing to do regardless. How do we best accomplish this manual enabling/disabling? |
| So does it work now or not? If, how to add it to the program? |
| AFAIK the Arctic controller is currently detected automatically. |
| For me it isnt. (Arctic LF3 360 Pro ARGB) It just stays white, no matter what zone im trying to change it stays unchanged. Same stuff on the same hub changes tho. But it cant be a Motherboard/Hardware problem since other programs like Mystic Light and Signal can detect it. |
| What kind of hub are you referring to? |
| Please note that the arctic RGB controller does not support addressable RGB (ARGB) devices. |
3 participants
This adds support for the Arctic RGB controller (product page).
For this controller to function, controlling DTR on the serial port is necessary.The first commit adds the necessary code
to the serial library, while the second commit fixes a configuration issue with serial ports under Linux which disrupted
the communication with the controller (maybe switch to QtSerialPort?). The following commit fixes a race condition
inside RGBController which would cause random crashes caused by the keepalive thread calling UpdateLEDs(). The last commit then adds support for the controller itself.
The whole series was tested on real hardware.