Added '-padding' option to uartdmx plugin. #1656

rbalykov · 2020-08-02T12:42:54Z

Sets minimal slots count in UART frame. Default is 0, will behave as before.

FloEdelmann

Please also add the setting to the plugin README

Sets minimal slots count in UART frame. Default is 0, will behave as before.

rbalykov · 2020-08-04T06:30:22Z

FAIL: common/network/HealthCheckedConnectionTester - do I need to fix it some way?
Missing from tarball plugins/uartdmx/README.md - i've checked it out from "master", edited and commited to "0.10". Need smth else?

peternewman · 2020-08-04T15:14:53Z

1. FAIL: common/network/HealthCheckedConnectionTester - do I need to fix it some way?

No, some of the tests area bit flaky on Mac, you can ignore this.

2. Missing from tarball plugins/uartdmx/README.md - i've checked it out from "master", edited and commited to "0.10". Need smth else?

On 0.10 the plugin description is in the code:

ola/plugins/uartdmx/UartDmxPlugin.cpp

Lines 114 to 120 in 7892723

    
           /** 
        
            * Return a description for this plugin. 
        
            */ 
        
           string UartDmxPlugin::Description() const { 
        
             return 
        
           "Native UART DMX Plugin\n" 
        
           "----------------------\n"

However I'd suggest if this code was to go in, that it should go in master, as it's a feature not a bug fix.

Can I ask the broader question, why? The web interface sends a full frame, any API can do so too, what's the reasoning for adding this to this plugin rather than dealing with it at source? I think the Enttec Pro's behave the same way and probably the Open's too. This would also be a perfect candidate for Patcher v2 #280.

rbalykov · 2020-08-04T15:55:30Z

Missing from tarball plugins/uartdmx/README.md - i've checked it out from "master", edited and commited to "0.10". Need smth else?
On 0.10 the plugin description is in the code:

So, keep README or edit in-code descriptption?
Plugin is still "Native UART", nothing has changed.

However I'd suggest if this code was to go in, that it should go in master, as it's a feature not a bug fix.
Can I ask the broader question, why?

This code fixes #1523 , I got some "hardware madness" with zero-lenght frames.
Sure, it could be fixed with ola_set_dmx, but this still gives some "broken" frames on boot.
Had another PR #1573 with same issue before, but was unfamiliar with github, so mixed two PRs in one commit. This one gives just "-padding" relates code.

peternewman · 2021-01-08T02:06:12Z

Sorry this slipped through the net a bit.

Missing from tarball plugins/uartdmx/README.md - i've checked it out from "master", edited and commited to "0.10". Need smth else?
On 0.10 the plugin description is in the code:

So, keep README or edit in-code descriptption?
Plugin is still "Native UART", nothing has changed.

Edit the in-code description if targetting 0.10 or the README if targetting master.

However I'd suggest if this code was to go in, that it should go in master, as it's a feature not a bug fix.
Can I ask the broader question, why?

This code fixes #1523 , I got some "hardware madness" with zero-lenght frames.

Ah okay, makes sense.

Sure, it could be fixed with ola_set_dmx, but this still gives some "broken" frames on boot.

Yeah I think we're agreeing, fixing in the plugin is the most sense.

Had another PR #1573 with same issue before, but was unfamiliar with github, so mixed two PRs in one commit. This one gives just "-padding" relates code.

Great thanks.

peternewman

Sorry this got missed before @rbalykov , thanks for your contribution, a few comments if you don't mind.

peternewman · 2021-01-08T02:07:03Z

plugins/uartdmx/README.md

@@ -0,0 +1,34 @@
+Native UART DMX Plugin


As discussed, please remove this file and edit PluginDescription if targeting 0.10.

peternewman · 2021-01-08T02:07:55Z

plugins/uartdmx/UartDmxDevice.h

@@ -60,11 +61,14 @@ class UartDmxDevice : public Device {
  const std::string m_path;
  unsigned int m_breakt;
  unsigned int m_malft;
+  unsigned int m_paddingt;


I think the t's were timing, e..g break time, mark after last frame time, so this should probably just be m_padding.

peternewman · 2021-01-08T02:17:32Z

plugins/uartdmx/UartWidget.cpp

  data.Get(buffer + 1, &length);
+  if (length < m_padding) {
+      memset((buffer + 1 + length), 0x00, (m_padding - length) );
+      length = m_padding;
+    }


Given as you've said this is probably required in a few other plugins, I wonder if it makes sense to add some extra options to DmxBuffer? e.g. GetWithPadding(uint8_t *data, unsigned int *length, unsigned int min_length). Or alternatively a Pad/Grow function which modifies the buffer in place (to save duplicating lots of functions), although the latter would need to be called on the lower level buffer in UartThread as this one is const so maybe GetWithPadding is simpler.

peternewman · 2021-01-08T02:18:29Z

plugins/uartdmx/UartWidget.h

@@ -48,9 +48,9 @@ class UartWidget {
    /**
     * Construct a new UartWidget instance for one widget.
     * @param path The device file path of the serial port
+     * @param padding Minimum DMX frame slots count, padded with NULLs


Possibly semantics, but I'd say they are zeros here, as they will be actual channel data.

peternewman · 2021-01-08T02:19:41Z

plugins/uartdmx/UartWidget.cpp

@@ -54,8 +55,9 @@ namespace uartdmx {
 using std::string;
 using std::vector;

-UartWidget::UartWidget(const string& path)
+UartWidget::UartWidget(const std::string &path, unsigned int padding)


The std:: isn't needed here as we're using std::string

peternewman · 2021-01-08T02:20:28Z

plugins/uartdmx/UartWidget.cpp

@@ -46,6 +46,7 @@
 #include "ola/io/IOUtils.h"
 #include "ola/Logging.h"
 #include "plugins/uartdmx/UartWidget.h"
+#include "plugins/uartdmx/UartDmxDevice.h"


I don't think we need this include do we?

peternewman · 2021-01-08T02:22:51Z

plugins/uartdmx/README.md

+The Mark After Last Frame time in microseconds for this device (optional).
+
+`<device>-padding = 0` Minimal slots count in DMX frame (optional). 
+Default is 0, which allows empty frame. Using minimal timing, frame period reaches 140 microseconds, while standard requires minimum 1204us. Using BREAK=88us and MAB=8us, frame has to include 25 slots, that gives frame period 1240us. 


If you're feeling particularly keen, it would be really nice on initialisation to print an OLA_INFO with the minimum padding value (based on break/MAB/MALF), or even an OLA_WARN if the configured padding is insufficient!

Added '-padding' option to uartdmx plugin.

4505ce0

Sets minimal slots count in UART frame. Default is 0, will behave as before.

FloEdelmann requested changes Aug 2, 2020

View reviewed changes

rbalykov and others added 5 commits August 3, 2020 19:40

Added '-padding' option to uartdmx plugin.

cfb0cb5

Sets minimal slots count in UART frame. Default is 0, will behave as before.

Merge branch '0.10' of https://github.com/rbalykov/ola into 0.10

ee95628

Fix travis errors in PR

3fa8156

Fix travis erros in PR

31d7a56

Fix indentation

7892723

rbalykov requested a review from FloEdelmann August 4, 2020 15:01

peternewman requested changes Jan 8, 2021

View reviewed changes

Merge branch 'OpenLightingProject:0.10' into 0.10

08e7e97

peternewman added this to the 0.10.9 milestone Dec 6, 2022

peternewman linked an issue Dec 6, 2022 that may be closed by this pull request

wrong output mode (UART-DMX, but affects virtually elsewhere) #1523

Open

Merge branch '0.10' into 0.10

8dad957

peternewman modified the milestones: 0.10.9, 0.10.10 Feb 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added '-padding' option to uartdmx plugin. #1656

Added '-padding' option to uartdmx plugin. #1656

rbalykov commented Aug 2, 2020

FloEdelmann left a comment

rbalykov commented Aug 4, 2020

peternewman commented Aug 4, 2020

rbalykov commented Aug 4, 2020 •

edited

Loading

peternewman commented Jan 8, 2021

peternewman left a comment

peternewman Jan 8, 2021

peternewman Jan 8, 2021

peternewman Jan 8, 2021

peternewman Jan 8, 2021

peternewman Jan 8, 2021

peternewman Jan 8, 2021

peternewman Jan 8, 2021

Added '-padding' option to uartdmx plugin. #1656

Are you sure you want to change the base?

Added '-padding' option to uartdmx plugin. #1656

Conversation

rbalykov commented Aug 2, 2020

FloEdelmann left a comment

Choose a reason for hiding this comment

rbalykov commented Aug 4, 2020

peternewman commented Aug 4, 2020

rbalykov commented Aug 4, 2020 • edited Loading

peternewman commented Jan 8, 2021

peternewman left a comment

Choose a reason for hiding this comment

peternewman Jan 8, 2021

Choose a reason for hiding this comment

peternewman Jan 8, 2021

Choose a reason for hiding this comment

peternewman Jan 8, 2021

Choose a reason for hiding this comment

peternewman Jan 8, 2021

Choose a reason for hiding this comment

peternewman Jan 8, 2021

Choose a reason for hiding this comment

peternewman Jan 8, 2021

Choose a reason for hiding this comment

peternewman Jan 8, 2021

Choose a reason for hiding this comment

rbalykov commented Aug 4, 2020 •

edited

Loading