Skip to content

Commit

Permalink
Merge pull request #13 from peternewman/patch-1
Browse files Browse the repository at this point in the history 
Untested fix to ensure we act on vendorcast/broadcast mute/unmute
  • Loading branch information
@mathertel
mathertel authored Aug 22, 2018
2 parents e664c6b + d546ea5 commit 63aa0e8
Showing 1 changed file with 8 additions and 4 deletions.
12 changes: 8 additions & 4 deletions src/DMXSerial2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ void DMXSerialClass2::tick(void)

} else if (Parameter == SWAPINT(E120_DISC_UN_MUTE)) { // 0x0003
isHandled = true;
if (packetIsForMe) { // 05.12.2013
if (packetIsForMe || packetIsForGroup || packetIsForAll) { // For us, process unmute
if (_rdm.packet.DataLength > 0) {
// Unexpected data
// Do nothing
Expand All @@ -532,13 +532,15 @@ void DMXSerialClass2::tick(void)
_rdm.packet.Data[0] = 0b00000000;
_rdm.packet.Data[1] = 0b00000000;
_rdm.packet.DataLength = 2;
respondMessage(true); // 21.11.2013
if (packetIsForMe) { // Only actually respond if it's sent direct to us
respondMessage(true); // 21.11.2013
}
}
}

} else if (Parameter == SWAPINT(E120_DISC_MUTE)) { // 0x0002
isHandled = true;
if (packetIsForMe) { // 05.12.2013
if (packetIsForMe || packetIsForGroup || packetIsForAll) { // For us, process unmute
if (_rdm.packet.DataLength > 0) {
// Unexpected data
// Do nothing
Expand All @@ -548,7 +550,9 @@ void DMXSerialClass2::tick(void)
_rdm.packet.Data[0] = 0b00000000;
_rdm.packet.Data[1] = 0b00000000;
_rdm.packet.DataLength = 2;
respondMessage(true); // 21.11.2013
if (packetIsForMe) { // Only actually respond if it's sent direct to us
respondMessage(true); // 21.11.2013
}
}
}

Expand Down

3 comments on commit 63aa0e8

@g0uus
Copy link

@g0uus g0uus commented on 63aa0e8 Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I have just spotted this issue (failure to unmute to broadcast address) and was about to tell you about it when I found this commit.

However, having fixed it in my own code, I believe the patch contains excessive logic.

Firstly, the test on lines 525 & 543

    if (packetIsForMe || packetIsForGroup || packetIsForAll) { // For us, process unmute

is already performed (using negative logic) near the start of the DMXSerialClass2::tick() function, we never even get to these lines if the test is false

      if ((! packetIsForMe) && (! packetIsForGroup) && (! packetIsForAll)) {
        // ignore this packet

      } else if (CmdClass == E120_DISCOVERY_COMMAND) { // 0x10

Secondly, there is no point in populating the _rdm.packet.Data and DataLength if we are not sending a response, thus instead of

              _isMute = false;
              // Control field
              _rdm.packet.Data[0] = 0b00000000;
              _rdm.packet.Data[1] = 0b00000000;
              _rdm.packet.DataLength = 2;
              if (packetIsForMe) { // Only actually respond if it's sent direct to us
                respondMessage(true); // 21.11.2013
              }

I believe we should have

              _isMute = false;
              // Control field
              if (packetIsForMe) { // Only actually respond if it's sent direct to us
                _rdm.packet.Data[0] = 0b00000000;
                _rdm.packet.Data[1] = 0b00000000;
                _rdm.packet.DataLength = 2;
                respondMessage(true); // 21.11.2013
              }

(and obviously the same change to the E120_DISC_MUTE branch.)

@peternewman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @g0uus ,

Your comment will probably get more visibility if you comment in #10 or #13 .

Well spotted, I'd agree with that review. Please feel free to open a pull request to fix my rather ropy code. I was mostly concentrating on correcting the initial suggestion in the bug report.

I'm still hoping to try and find some time to add checks to the OLA RDM tests to validate these code changes for real.

@g0uus
Copy link

@g0uus g0uus commented on 63aa0e8 Oct 5, 2018 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.