Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Getting Dimmer Max Value #1561

Merged
merged 6 commits into from
Jun 30, 2019

Conversation

kecramer
Copy link
Contributor

Dimmer max is a single 16 bit value, not an 8 bit.

@kecramer kecramer closed this Jun 14, 2019
@kecramer kecramer reopened this Jun 14, 2019
@kecramer
Copy link
Contributor Author

kecramer commented Jun 14, 2019

This is failing make check for me locally but so is master, and I believe it unrelated to this code.

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

One comment.

What's the error you get with make check?

common/rdm/RDMAPI.cpp Outdated Show resolved Hide resolved
@kecramer
Copy link
Contributor Author

kecramer commented Jun 14, 2019

common/utils/DmxBufferTest.cpp:197:10: error: explicitly assigning value of
      variable of type 'ola::DmxBuffer' to itself
      [-Werror,-Wself-assign-overloaded]
  buffer = buffer;
  ~~~~~~ ^ ~~~~~~
1 error generated.

Is the error when I am up to date with origin/master (and for this branch as well)

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Hi @kecramer ,

Looking at this a bit more, please can you make a GenericGet and Handle equivalent to GenericGetU32/_HandleU32Response for U16 and change this and GetDMXAddress to use it please.

It, and the GetU8 and U32 variants should also all have the CheckCallback line:

 if (CheckCallback(error, callback))
   return false;

@peternewman
Copy link
Member

Thanks @kecramer . Sorry I missed it before, but could you switch SetDMXAddress to use the new GenericSetU16 you've very sensibly added.

@kecramer
Copy link
Contributor Author

Ah good point, definitely missed that one. Let me know if you have any other comments.

@peternewman
Copy link
Member

Ah good point, definitely missed that one. Let me know if you have any other comments.

Well in fairness I didn't initially suggest even making a generic set as I'd missed the fact the brightness would have a set too.

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this bugfix and the related tidying.

@peternewman peternewman merged commit 41d4201 into OpenLightingProject:master Jun 30, 2019
@peternewman peternewman added this to the 0.11.0 milestone Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants