Initial FTDI RDM support #1541

Keeper-of-the-Keys · 2019-02-26T12:13:41Z

The support is not 100% according to the standard, it for instance does not check if after the minimum wait duration a full packet was actually received and if not waits longer.
This should of course only be a problem for devices that for some reason don't manage to send an RDM packet within normal/optimal timings.

I figure that adding that logic/better timings should be part of a separate later PR.

This will probably replace Write(DmxBuffer) in the future and maybe should return bool.

…or logic to prevent memory leaks (I hope).

…rewrite/undo a lot of my changes but figure I should maintain stuff just in case.

…or ftdidmx, code still very ugly and hacky and olad doesn't shut down properly.

…tput to FtdiDmxThread::SendRDMRequest()

It still doesn't work and some behaviors are plain wrong (the callbacks being called when stuff can't get to the line) Sadly my second FTDI interface is broken so I can't currently check if RDM packets are actually hitting the line or not. Also still leaves olad that doesn't shutdown without kill -9.

Added the DiscoveryAgent and inheritance from DiscoverableRDMControllerInterface

…sted

At this point only UnMuteAll gets called, after its' callback is fired no further RDM packets are sent. Also I'm pretty sure I may be leaking some memory.

Added some more debug output for when an operation is pending/

…more debug output. Works now, but reply is not being read I really need to get another FTDI interface or some other way to monitor the line :/ Also shutdown works again (which may be due to next commit)

… switching messing up the reading of the reply. Still no luck, will need to wait for new FTDI board to read the line.

1. Clear packetbuffer before use. 2. Fix logic of callbacks (else was to wrong if()) 3. Fix packet reconstruction for non-DUB replies: the MAB is recognized as another byte by the FTDI so it reads 1 byte more then the actual packet so we hand of starting 1 byte later and 1 byte less total.

…cks. This declutters rest of the code.

It runs them, the pending request should be destroyed by the core that generated it which is why it can be set to null.

Old scheduling logic commented out, will be removed in next commit.

peternewman

A few initial notes to get you started...

.gitignore

plugins/ftdidmx/FtdiDmxPort.h

plugins/ftdidmx/FtdiDmxThread.cpp

peternewman · 2019-02-26T12:32:16Z

Travis lint errors will need fixing too:
https://travis-ci.org/OpenLightingProject/ola/jobs/498695471

peternewman · 2019-02-26T12:33:34Z

And this compiler error:

libtool: compile:  ccache g++-8 -DHAVE_CONFIG_H -I. -I../.. -Wno-deprecated-declarations -I../../include -I./include -Wall -Wformat -W -fvisibility-inlines-hidden -pthread -Werror -Wno-error=deprecated-declarations -g -O2 -std=gnu++11 -pthread -MT plugins/ftdidmx/FtdiDmxThread.lo -MD -MP -MF plugins/ftdidmx/.deps/FtdiDmxThread.Tpo -c ../../plugins/ftdidmx/FtdiDmxThread.cpp -o plugins/ftdidmx/FtdiDmxThread.o >/dev/null 2>&1
../../plugins/ftdidmx/FtdiWidget.cpp: In member function â€˜bool ola::plugin::ftdidmx::FtdiInterface::Write(ola::io::ByteString*)â€™:
../../plugins/ftdidmx/FtdiWidget.cpp:373:61: error: invalid conversion from â€˜const unsigned char*â€™ to â€˜unsigned char*â€™ [-fpermissive]
   int bytesWritten = ftdi_write_data(&m_handle, packet->data(), packet->size());
                                                 ~~~~~~~~~~~~^~
In file included from ../../plugins/ftdidmx/FtdiWidget.cpp:42:
/usr/include/ftdi.h:406:67: note:   initializing argument 2 of â€˜int ftdi_write_data(ftdi_context*, unsigned char*, int)â€™
     int ftdi_write_data(struct ftdi_context *ftdi, unsigned char *buf, int size);
                                                    ~~~~~~~~~~~~~~~^~~

peternewman · 2019-02-26T14:48:41Z

It works though!!! I've just plugged in an FTDI RS485 lead and done some discovery and identify! 🎆 🍾 💯 👍

peternewman · 2019-02-26T14:49:33Z

And this compiler error:

libtool: compile:  ccache g++-8 -DHAVE_CONFIG_H -I. -I../.. -Wno-deprecated-declarations -I../../include -I./include -Wall -Wformat -W -fvisibility-inlines-hidden -pthread -Werror -Wno-error=deprecated-declarations -g -O2 -std=gnu++11 -pthread -MT plugins/ftdidmx/FtdiDmxThread.lo -MD -MP -MF plugins/ftdidmx/.deps/FtdiDmxThread.Tpo -c ../../plugins/ftdidmx/FtdiDmxThread.cpp -o plugins/ftdidmx/FtdiDmxThread.o >/dev/null 2>&1
../../plugins/ftdidmx/FtdiWidget.cpp: In member function â€˜bool ola::plugin::ftdidmx::FtdiInterface::Write(ola::io::ByteString*)â€™:
../../plugins/ftdidmx/FtdiWidget.cpp:373:61: error: invalid conversion from â€˜const unsigned char*â€™ to â€˜unsigned char*â€™ [-fpermissive]
   int bytesWritten = ftdi_write_data(&m_handle, packet->data(), packet->size());
                                                 ~~~~~~~~~~~~^~
In file included from ../../plugins/ftdidmx/FtdiWidget.cpp:42:
/usr/include/ftdi.h:406:67: note:   initializing argument 2 of â€˜int ftdi_write_data(ftdi_context*, unsigned char*, int)â€™
     int ftdi_write_data(struct ftdi_context *ftdi, unsigned char *buf, int size);
                                                    ~~~~~~~~~~~~~~~^~~

This fixes it, and works for me:
int bytesWritten = ftdi_write_data(&m_handle, const_cast<unsigned char*>(packet->data()), packet->size());

peternewman · 2019-02-26T19:21:13Z

Also can you update the FTDI plugin's readme please, probably with a general note, and a list of known good, and known bad devices, where it works or doesn't (e.g. the likely biasing issue on the Enttec Open, although I never could find the circuit diagram the other day to confirm).

plugins/ftdidmx/FtdiDmxThread.cpp

…r seems more logical at hw level so moved there. Thread level function still commented out in code will be removed later. The thread does not seem to need to be echo aware, however once I do tests we'll know for sure.

… is ON. Also fixed debug output targets to right targets. TODO: Add echo detection to setup. TODO: Convert Write(DmxBuffer) to use Write(ola::io::ByteString)

As mentioned on IRC it seems to me that this behavior is not desirable.

…assed.

far causes crashes because Size() is too often 0 instead of within allowed range.

TODO: Update docs and run through valgrind.

Valgrind results suggests no leak in plugin, however there do seem to be leaks in RDM Core.

…null packets also with only 24 slots of data.

…d list into variable. Also one spelling correction I missed.

peternewman

Some more comments

peternewman · 2019-10-09T14:15:57Z

common/utils/Clock.cpp

          m_tv.tv_usec / ONE_THOUSAND);
 }

+int64_t BaseTimeVal::InMicroSeconds() const {


This returns exactly the same value as BaseTimeVal::AsInt(), any reason we need the second function?

This is returning msec not usec...

I guess AsInt is too then; I've just double-checked and the code is identical in both, only the method name changes.

peternewman · 2019-10-09T14:19:00Z

common/utils/Clock.cpp

+  timespec req;
+  req.tv_sec = requested / USEC_IN_SECONDS;
+  req.tv_nsec = (requested % USEC_IN_SECONDS) * ONE_THOUSAND;
+  req.tv_sec = 0;


Why is this assigned and then reset?

I suspect an error on my part, though I also have a suspicion that maybe I did not want this function to exceed 1s

peternewman · 2019-10-09T14:22:44Z

common/utils/StringUtils.cpp

+bool StringToInt(const string &value,
+                 unsigned int *output,
+                 bool strict,
+                 uint8_t base) {


Like the stuff in https://github.com/OpenLightingProject/ola/blob/master/common/utils/StringUtilsTest.cpp#L541-L564 for example.

So passing in something in base 36, and also validating that 123ABC converts to the same value as 123abc.

peternewman · 2019-10-09T14:24:28Z

common/utils/StringUtils.cpp

    return false;
  }
+
+  *output = static_cast<unsigned int>(l);


But if we've returned false we shouldn't bother setting the value.

peternewman · 2019-10-09T14:27:48Z

plugins/ftdidmx/FtdiDmxThread.h

-class FtdiDmxThread : public ola::thread::Thread {
+enum {
+  HALF_SECOND_MS = 500,
+  ALMOST_SECOND_MS = 900,


Keeper-of-the-Keys · 2023-12-04T18:16:11Z

@peternewman I may want to retry getting this in, I suspect it will be better to do a new PR against the current branch and maybe also smaller PRs (in as much as that is possible) that add differnent features, WDYT?

CaptChrisD · 2025-02-03T22:03:56Z

@Keeper-of-the-Keys I -- as well as others -- am also interested in getting this in. Any chance some effort could be put towards this.

Keeper-of-the-Keys · 2025-02-03T22:31:58Z

@Keeper-of-the-Keys I -- as well as others -- am also interested in getting this in. Any chance some effort could be put towards this.

I tried picking it back up but it turns out I broke one of my FTDI boards, I don't really have budget at the moment to buy new boards and thus it got snowed under other projects that pay my bills at the moment.

Specifically this PR is too large to get merged IMHO and my plan was to start working on the different components and get them merged one by one but as said need working hardware to develop software.

That being said I do still hope to get to it...

CaptChrisD · 2025-02-03T22:34:45Z

@Keeper-of-the-Keys would you mind reaching out to me? chris at bigcove.io

E.S. Rosenberg a.k.a. Keeper of the Keys added 24 commits December 3, 2018 01:22

Merge remote-tracking branch 'upstream/master' into 0.10-libftdi1

bfad65f

Added overload of Write() with void Write(ola::io::ByteString)

78cc4b9

This will probably replace Write(DmxBuffer) in the future and maybe should return bool.

Indicate that RDM is supported

e1869e3

Initial attempt at adding queue for RDM commands, also added destruct…

2b5a5fa

…or logic to prevent memory leaks (I hope).

This a totally disgusting in-between state of the code, I'm about to …

3b6cb83

…rewrite/undo a lot of my changes but figure I should maintain stuff just in case.

Initial compiling version that implements a RDM discoverable target f…

c51928e

…or ftdidmx, code still very ugly and hacky and olad doesn't shut down properly.

Removed qtcreator files, added to .gitignore and added minor debug ou…

10fc5a0

…tput to FtdiDmxThread::SendRDMRequest()

Merge remote-tracking branch 'upstream/master' into ftdi-rdm

71bd693

Another attempt with no fruits...

8553598

Added the DiscoveryAgent and inheritance from DiscoverableRDMControllerInterface

Added discovery functions to port, now stuff actually gets called.

81d120b

Some debug output plus detect that less bytes were written then reque…

8914f56

…sted

Some logic fixes that came up now that discovery actually tries to run.

9e95824

At this point only UnMuteAll gets called, after its' callback is fired no further RDM packets are sent. Also I'm pretty sure I may be leaking some memory.

Clean up Port and remove functions not needed.

ff09297

Minor cosmetic change

851401b

Added some of the RDMReply callback logic.

1d09fe1

Added some more debug output for when an operation is pending/

Set the pending request back to nullptr in the right places and nome …

b02363b

…more debug output. Works now, but reply is not being read I really need to get another FTDI interface or some other way to monitor the line :/ Also shutdown works again (which may be due to next commit)

Added FtdiInterface::WriteAndRead() in an attempt to rule out context…

c87ebf6

… switching messing up the reading of the reply. Still no luck, will need to wait for new FTDI board to read the line.

Added single function to handle destruction of any outstanding callba…

2373f60

…cks. This declutters rest of the code.

Added 'destruction' of callbacks to destructor.

2198e3d

It runs them, the pending request should be destroyed by the core that generated it which is why it can be set to null.

Switch back to scheduling with seperate write() and read() functions.

65e11d8

Old scheduling logic commented out, will be removed in next commit.

Removal of WriteAndRead() and related scheduler.

d719f6b

Merge remote-tracking branch 'upstream/master' into ftdi-rdm

5645a01

peternewman requested changes Feb 26, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into ftdi-rdm

c640e4e

peternewman reviewed Jul 6, 2019

View reviewed changes

plugins/ftdidmx/FtdiDmxThread.cpp Outdated Show resolved Hide resolved

E.S. Rosenberg a.k.a. Keeper of the Keys added 21 commits July 28, 2019 18:16

Switch from srand to ola::math::Random()

c2083af

Add LOG/DEBUG outputs to echo detection

717462d

Add logic to Write(ola::io::ByteString) to skip written bytes if echo…

7b0ecf8

… is ON. Also fixed debug output targets to right targets. TODO: Add echo detection to setup. TODO: Convert Write(DmxBuffer) to use Write(ola::io::ByteString)

Disable auto discovery.

ed1eb6b

As mentioned on IRC it seems to me that this behavior is not desirable.

Allow RDM to continue if no pending DMX packet and less then second p…

5392ab0

…assed.

Attempt at making Write(DmxBuffer) use Write(ola::io::ByteString) so

c666d71

far causes crashes because Size() is too often 0 instead of within allowed range.

Fix description string to actually reflect what port is being used.

249d826

Now works regardless of echo state and biasing resistors.

bfc14db

TODO: Update docs and run through valgrind.

Updated docs to reflect state of driver.

48d1dbe

Valgrind results suggests no leak in plugin, however there do seem to be leaks in RDM Core.

Fix some Travis spellintian issues

a198e63

Merge remote-tracking branch 'upstream/master' into ftdi-rdm

368a2f1

Pad packets smaller then 24 slots to 24 slots, if forced to send out …

e80f64d

…null packets also with only 24 slots of data.

Fix spelling errors found by 'codespell' Travis-CI check.

011f591

Added FtdiDmxPluginDescription.h to lint test blacklist and moved sai…

b1dc145

…d list into variable. Also one spelling correction I missed.

lint file search expression fixed?

a0d202a

lint find statement fixed

4566ff8

lint find statement

c85f065

Another attempt for lint files

e48cebd

fix wrong bracket in lint

14a4513

Another attempt

117f831

peternewman requested changes Oct 9, 2019

View reviewed changes

Merge branch 'master' into ftdi-rdm

1fb4bda

Keeper-of-the-Keys mentioned this pull request Jan 25, 2020

Simplify the sed command used to generate plugin header files, closes #1592 (hopefully) #1608

Merged

Initial FTDI RDM support #1541

Are you sure you want to change the base?

Initial FTDI RDM support #1541

Conversation

Keeper-of-the-Keys commented Feb 26, 2019

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peternewman commented Feb 26, 2019

Uh oh!

peternewman commented Feb 26, 2019

Uh oh!

peternewman commented Feb 26, 2019

Uh oh!

peternewman commented Feb 26, 2019

Uh oh!

peternewman commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Keeper-of-the-Keys commented Dec 4, 2023

Uh oh!

CaptChrisD commented Feb 3, 2025

Uh oh!

Keeper-of-the-Keys commented Feb 3, 2025

Uh oh!

CaptChrisD commented Feb 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peternewman commented Feb 26, 2019 •

edited

Loading