SPI DMX receiver by FloEdelmann · Pull Request #1289 · OpenLightingProject/ola

FloEdelmann · 2017-07-21T14:29:24Z

Closes #1285, see there for a description.

This is still a work in progress, I just want Travis to run its tests. On my laptop I compile and run it fine. On the Raspberry, even with a fresh autoreconf, after compiling and installing, running olad gives

olad: error while loading shared libraries: libolaspidmx.so.0: cannot open shared object file: No such file or directory

I'm confused since it works on my laptop, so I can't have missed something, right?

EDIT: Apparently, Travis does also only fail due to a mistake in protobuf. spidev is actually not available on Mac so it is skipped completely.

FloEdelmann · 2017-07-21T19:00:09Z

It basically works! 🎉

I had to manually set the LD_LIBRARY_PATH environment variable to /usr/local/lib; it was empty before. Any idea how that could have happened?

I still have to look into an occasionally happening segfault because it tries to free already freed memory. Also, documentation is still incomplete. But apart from that, I'm finished :)

FloEdelmann · 2017-07-22T23:08:18Z

Now it's finished. I'd be happy to see you review it :)

peternewman

A quick one for starters, can you fix the lint and Doxygen failures on Travis please @FloEdelmann . I'll try and find some time for a proper review soon.

peternewman

An initial review

peternewman · 2017-08-11T02:26:07Z

+  }
+
+  bool save = m_preferences->SetDefaultValue(DeviceBlocklength(),
+                                             UIntValidator(0, 65535),


Surely 0 as a minimum value is nonsense, you want at least 1 slot don't you?

peternewman · 2017-08-11T02:28:24Z

+  while (chunk_bitcount < buffersize) {
+    switch (state) {
+      case WAIT_FOR_BREAK:
+        // printf("%6ld  0x%02x  wait for break (%d)\n", chunk_bitcount,


Can we remove this debug please, or convert it to OLA_DEBUG.

If I put it in OLA_DEBUG, it outputs so much that receiving gets too laggy to be usable. I'll remove those comments shortly before merging but I keep them until then for easy debugging.

peternewman · 2017-08-11T02:31:12Z

+
+/**
+ * Helper function that returns the number of zeros if a falling edge is
+ * detected in the given byte or -1 if the byte is no falling edge.


SPaG "is not a falling edge" is I think what you're trying to say?

peternewman · 2017-08-11T02:33:00Z

+
+/**
+ * Helper function that returns the number of ones if a rising edge is
+ * detected in the given byte or -1 if the byte is no rising edge.


peternewman · 2017-08-11T02:36:54Z

+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * SpiDmxPlugin.cpp
+ * This looks for possible SPI devices to instanciate and is managed by OLAD.


SPaG instantiate

peternewman · 2017-08-11T02:41:51Z

+        m_plugin_adaptor(plugin_adaptor) {
+  }
+
+  ola_plugin_id Id() const { return OLA_PLUGIN_EXPERIMENTAL; }


Can you define a new OLA_PLUGIN_SPIDMX in https://github.com/OpenLightingProject/ola/blob/master/common/protocol/Ola.proto and set the value to 10000 for now, then we'll confirm an actual value when it gets merged. But doing it that way will increase the chances we'll remember to do so!

When makeing, I get the following error:

"ola.proto.OLA_PLUGIN_EXPERIMENTAL" uses the same enum value as "ola.proto.OLA_PLUGIN_SPIDMX". If this is intended, set 'option allow_alias = true;' to the enum definition.

Should I add the suggested option somewhere (where?) or just set the constant to 9999?

Interestingly this only happens on the OS X builds, which I think use a later protobuf.

As you can probably tell, we've only been through this particular workflow a few times with PRs. Essentially what we're trying to do initially is stopping you grabbing 23 for example and someone else having it too, as well as ensuring your code doesn't just use the OLA_PLUGIN_EXPERIMENTAL constant. I guess we could either comment out OLA_PLUGIN_EXPERIMENTAL (which will kill your same enum value error, or perhaps just change the comment there to say an ID greater than OLA_PLUGIN_EXPERIMENTAL, and allow a free for all up there, knowing they'll get updated before merging.

As a PR submitter, which one makes sense/is most clear to you? How can we reword the code comment so you'd do that from the off, rather than what you interpreted it as?

Do we need the OLA_PLUGIN_EXPERIMENTAL then at all? The comment could just say that all IDs above 10000 are experimental and used during development until a fixed IDs is assigned.

Yeah you're right, we probably don't. Hopefully that will make it less confusing for people too. Do you fancy trying to update the words for this, or would you rather I did it?

I've made an attempt in b8ecf02. Is this okay?

peternewman · 2017-08-11T02:43:59Z

+void SpiDmxThread::RegisterPort() {
+  m_registered_ports++;
+
+  if (m_registered_ports == 1) {


= 1 perhaps, although it shouldn't happen.

Well done for understanding what I meant, even when the GitHub markdown confused it!

peternewman · 2017-08-11T02:44:28Z

+void SpiDmxThread::UnregisterPort() {
+  m_registered_ports--;
+
+  if (m_registered_ports == 0) {


<= 0 again just in case.

peternewman

So apologies again, as per the comment in #1286 (review) , can we change the C++ classes to be called SPIDMX...?

peternewman · 2017-09-05T01:45:59Z

+
+`<device>-blocklength = 4096`  
+How many SPI bytes (= DMX bits) should be received (optional). The default
+is 4096, but 8192 is recommended.


Out of interest, how come we don't default to the value we recommend?

On the Raspberry Pi, a 4kB read / transmit buffer is available without further configuration. If one wants to read more than that in a single call, the buffer size needs to be increased via a command line call. The default configuration should work without this call, though.

~~Ah yes, a few lines up the same file. I wonder if it's worth an N.B. here when recommending 8192 that they'll need to do some local config to enable this.~~ I see you've already done this...

Can you also detect and log that there's a mismatch based on the number of bytes returned when you do the reads, to alert the user their config isn't correct?

The call used to read / write (ioctl, in SPIDMXWidget.cpp) does not return the number of received / transmitted bytes but only an unspecified non-negative integer or -1 in an error case.

Since the call fails if the buffer is not big enough, the check should already be enough, I guess.

That's a shame. Perhaps the error on the line below needs a hint about buffer sizes though, if changing the value to 4097 will cause it to break?

Like this? (8724254)

peternewman · 2017-09-11T14:50:41Z

   * Plugin IDs are usually assigned just prior to merging the code into the
-   * mainline. For development of plugins please use the value of
-   * OLA_PLUGIN_EXPERIMENTAL in a plugin ID you define above.
+   * mainline. During development of new plugins, please use a value of 10000


We probably want this to be values of 10000 or more (so e.g. "a value of 10000 or greater")

peternewman · 2017-09-11T14:53:01Z

+
+  int ret = ioctl(m_fd, SPI_IOC_MESSAGE(1), &tr);
+  if (ret < 1) {
+    OLA_WARN << Name() << " ioctl read/write error. Maybe this is due to a"


Perhaps say "This may be due to a buffer size mismatch" or something similar.

peternewman · 2017-09-12T13:20:17Z

+
+`device_prefix = <string>`  
+The prefix of files to match in `/dev`. Usually set to `spidev`. Each match
+will instantiate a device.


You should add a note here that multiple device_prefix lines are supported.

peternewman · 2017-09-12T13:21:51Z

+}
+
+/**
+ * This thread does only have to run if ports using it are patched to a


SPaG "This thread only has to run if..."

peternewman · 2017-09-12T13:24:50Z

+  * @param callback The callback to call.
+  */
+bool SPIDMXThread::SetReceiveCallback(Callback0<void> *callback) {
+  if (!callback) {


Shouldn't this either be m_receive_callback, or the line below do a reset to NULL?

Oh right, thanks!

No worries, done.

peternewman · 2017-09-12T13:27:39Z

+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * SPIDMXWidget.cpp
+ * This is a wrapper around the needed SPIDEV calls.


SPaG "the required SPIDEV"

peternewman · 2017-09-12T13:28:37Z

+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * SPIDMXWidget.h
+ * This is a wrapper around the needed SPIDEV calls.


peternewman

Just a few more minor changes from me please.

peternewman · 2017-09-19T03:43:35Z

LGTM, over to @nomis52 .

FloEdelmann · 2017-10-16T10:00:22Z

Any news on this?

peternewman · 2017-10-16T18:10:06Z

@nomis52 have you got any time to review this please?

FloEdelmann · 2017-10-31T09:13:34Z

Since this PR will be referenced in my Bachelor's Thesis due next month, it would be nice if it could already be merged then :)

nomis52

LGTM bar the one comment.

nomis52 · 2017-11-02T03:51:25Z

  OLA_PLUGIN_UARTDMX = 20;
  OLA_PLUGIN_OPENPIXELCONTROL = 21;
  OLA_PLUGIN_GPIO = 22;
+  OLA_PLUGIN_SPIDMX = 10001;


If we're merging to mainline this should have a non-dev code.

Well spotted!

FloEdelmann · 2017-11-02T21:33:00Z

Nice, thank you! :)

peternewman · 2017-11-03T11:42:30Z

No worries, thanks for the code @FloEdelmann .

Coverity is complaining about:

** CID 185518:  Uninitialized members  (UNINIT_CTOR)
/plugins/spidmx/SPIDMXParser.h: 38 in ola::plugin::spidmx::SPIDMXParser::SPIDMXParser(ola::DmxBuffer *, ola::Callback0<void> *)()


________________________________________________________________________________________________________
*** CID 185518:  Uninitialized members  (UNINIT_CTOR)
/plugins/spidmx/SPIDMXParser.h: 38 in ola::plugin::spidmx::SPIDMXParser::SPIDMXParser(ola::DmxBuffer *, ola::Callback0<void> *)()
32
33     class SPIDMXParser {
34      public:
35       SPIDMXParser(DmxBuffer *buffer, Callback0<void> *callback)
36         : m_dmx_buffer(buffer),
37           m_callback(callback) {
>>>     CID 185518:  Uninitialized members  (UNINIT_CTOR)
>>>     Non-static class member "sampling_position" is not initialized in this constructor nor in any functions that it calls.
38       }
39       void ParseDmx(uint8_t *buffer, uint64_t chunksize);
40       void SetCallback(Callback0<void> *callback) {
41         m_callback = callback;
42       }
43

I've not looked carefully enough to realise why it's only moaning about sampling_position and not the other members, but would you mind resolving this in a new PR please @FloEdelmann ?

FloEdelmann added 5 commits July 19, 2017 14:57

add readme

f5b0301

bare plugin implementation

5a51247

SpiDmx plugin *should* work as expected now

792ec4e

Update copyright notices and file descriptions

cf89880

fix lint

321bf2f

FloEdelmann added 2 commits July 22, 2017 22:37

Double memory freeing should be fixed, remove many debug logs

0a8d1a3

cleanup, add doc comments

d4edaa3

FloEdelmann changed the title ~~[WIP] SPI DMX receiver~~ SPI DMX receiver Jul 22, 2017

peternewman requested changes Aug 8, 2017

View reviewed changes

fix lint, doxygen

3ec3602

peternewman requested changes Aug 11, 2017

View reviewed changes

FloEdelmann added 2 commits September 4, 2017 17:54

fix requested changes

8dba246

Merge remote-tracking branch 'upstream/master' into spidmx

76f80a7

peternewman requested changes Sep 5, 2017

View reviewed changes

FloEdelmann added 6 commits September 5, 2017 14:21

some smaller texts

af904e8

change SPIDMX plugin ID

832c1d7

rename SpiDmx to SPIDMX

de02244

change plugin ID comment

b8ecf02

Fix segfault when unpatching

17d2012

better help texts

8724254

peternewman reviewed Sep 11, 2017

View reviewed changes

FloEdelmann added 2 commits September 11, 2017 17:11

improve wording again

66d2a7a

Merge branch 'master' into spidmx

91665fa

peternewman reviewed Sep 12, 2017

View reviewed changes

peternewman assigned nomis52 Sep 12, 2017

peternewman requested changes Sep 12, 2017

View reviewed changes

FloEdelmann added 2 commits September 12, 2017 15:52

fix requested changes

77f8a15

remove debug comments

9b4710c

peternewman approved these changes Sep 19, 2017

View reviewed changes

peternewman requested a review from nomis52 September 19, 2017 03:43

peternewman and others added 3 commits September 27, 2017 17:34

Merge branch 'master' into spidmx

c2459b9

Merge branch 'master' into spidmx

03b7e70

Typo

b4194fe

peternewman assigned nomis52 and unassigned nomis52 Oct 16, 2017

peternewman assigned nomis52 and unassigned nomis52 Oct 27, 2017

peternewman added this to the 0.11.0 milestone Oct 28, 2017

nomis52 reviewed Nov 2, 2017

View reviewed changes

Update the plugin ID

07cc4d5

nomis52 approved these changes Nov 2, 2017

View reviewed changes

peternewman merged commit f9e7a76 into OpenLightingProject:master Nov 2, 2017

FloEdelmann deleted the spidmx branch November 2, 2017 21:33

FloEdelmann mentioned this pull request Nov 4, 2017

Initialize members of SPIDMXParser class #1329

Merged

peternewman mentioned this pull request Jul 12, 2026

Add 0.11 news #2060

Draft

Uh oh!

Conversation

FloEdelmann commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FloEdelmann commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FloEdelmann commented Jul 22, 2017

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

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!

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!

peternewman Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

peternewman Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FloEdelmann Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peternewman Sep 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

FloEdelmann Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

FloEdelmann commented Jul 21, 2017 •

edited

Loading

FloEdelmann commented Jul 21, 2017 •

edited

Loading

peternewman Sep 5, 2017 •

edited

Loading

peternewman Sep 5, 2017 •

edited

Loading

FloEdelmann Sep 5, 2017 •

edited

Loading

peternewman Sep 9, 2017 •

edited

Loading

FloEdelmann Sep 11, 2017 •

edited

Loading