Skip to content

Implement DIMMER_INFO, DIMMER_MINIMUM, and DIMMER_MAXIMUM PIDs per E1.37-1 - #1494

Merged
peternewman merged 26 commits into
OpenLightingProject:masterfrom
kecramer:dimmer-pids
Jan 16, 2019
Merged

Implement DIMMER_INFO, DIMMER_MINIMUM, and DIMMER_MAXIMUM PIDs per E1.37-1#1494
peternewman merged 26 commits into
OpenLightingProject:masterfrom
kecramer:dimmer-pids

Conversation

@kecramer

Copy link
Copy Markdown
Contributor

Includes code from #1483

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks.

The conflicts are sorted, but the comments haven't been addressed. Can you merge in the contents of this commit (and do the same fixes on the other functions).

Comment thread common/rdm/RDMAPI.cpp Outdated
Comment thread common/rdm/RDMAPI.cpp Outdated
Comment thread common/rdm/RDMAPI.cpp Outdated
Comment thread common/rdm/RDMAPI.cpp Outdated
Comment thread common/rdm/RDMAPI.cpp Outdated
Comment thread olad/RDMHTTPModule.cpp Outdated
Comment thread olad/RDMHTTPModule.cpp
Comment thread olad/RDMHTTPModule.cpp Outdated
Comment thread olad/RDMHTTPModule.cpp Outdated
Comment thread olad/RDMHTTPModule.cpp Outdated

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some more issues, mostly just minor code styling

Comment thread common/rdm/RDMAPI.cpp
Comment thread common/rdm/RDMAPI.cpp
Comment thread common/rdm/RDMAPI.cpp
Comment thread common/rdm/RDMAPI.cpp Outdated
Comment thread common/rdm/RDMAPI.cpp Outdated
Comment thread common/rdm/RDMAPI.cpp Outdated
Comment thread common/rdm/RDMAPI.cpp Outdated
Comment thread common/rdm/RDMAPI.cpp
Comment thread olad/RDMHTTPModule.h Outdated
Comment thread olad/RDMHTTPModule.h Outdated
@peternewman

Copy link
Copy Markdown
Member

I've fixed the Travis Codespell issue as it's just whitelisting, but can you fix the Travis lint issue please, two lines are > 80 characters.

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few more comments

Comment thread common/rdm/RDMAPI.cpp Outdated
Comment thread common/rdm/RDMAPI.cpp Outdated
Comment thread include/ola/rdm/RDMAPI.h Outdated
Comment thread common/rdm/RDMAPI.cpp Outdated

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hopefully this is almost it now. Have you tested a round trip of get/set/get against our dummy responders for the new PIDs you can set?

Comment thread olad/RDMHTTPModule.cpp Outdated
Comment thread olad/RDMHTTPModule.cpp
Comment thread include/ola/rdm/RDMAPI.h Outdated
Comment thread include/ola/rdm/RDMAPI.h Outdated
Comment thread olad/RDMHTTPModule.cpp Outdated
@kecramer

Copy link
Copy Markdown
Contributor Author

I have checked round trips and the behavior is the same as the command-line API. Maximum value setting has some odd behavior that is present in both - I haven't gotten to look into that yet, however.

@peternewman

Copy link
Copy Markdown
Member

Okay thanks, if you can give some detail on the maximum value oddities, I can try and make sense of it from the RDM side, it could be a bug in our implementation (and hence also the RDM tests if they didn't catch it).

@kecramer

Copy link
Copy Markdown
Contributor Author

So setting a maximum value should set that value, however the value that ends up getting set is not the value entered. For instance, setting the value to '60000' results in '65514' as the new value.

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One minor typo

Comment thread olad/RDMHTTPModule.cpp Outdated
@peternewman

peternewman commented Sep 28, 2018

Copy link
Copy Markdown
Member

So setting a maximum value should set that value, however the value that ends up getting set is not the value entered. For instance, setting the value to '60000' results in '65514' as the new value.

I've just tried that against master, and it works fine for me. Was this the dummy advanced dimmer responder, UID 7a70:ffffff04, or a real device?

Can you paste the exact ola_rdm_get/set commands you used, and what version everything reports? Were you using your locally compiled ola_rdm_get/set, or other ones (which version, installed how)?

Does it happen every time or just intermittently?

@kecramer

Copy link
Copy Markdown
Contributor Author

I've just tested it again on my branch and can't reproduce that either (from command line). Let me investigate what's going on, but definitely don't merge this yet. I wonder if the high order byte is always sending ff for some reason.

@peternewman

Copy link
Copy Markdown
Member

I've just tested it again on my branch and can't reproduce that either (from command line). Let me investigate what's going on, but definitely don't merge this yet. I wonder if the high order byte is always sending ff for some reason.

Very strange. I think you're using our HostToNetwork functions already anyway, a byte getting stuck seems odd too, I could just about imagine it referencing the wrong one, but then it would always be wrong.

@peternewman peternewman added this to the 0.11.0 milestone Oct 15, 2018
@peternewman

Copy link
Copy Markdown
Member

Did you get a chance to look further at this @kecramer ?

Given neither of us could reproduce, and the code all looks fine, I'm tempted to carry on and see where we are code review wise.

@kecramer

Copy link
Copy Markdown
Contributor Author

I tried to figure out what was going on and ended up spinning my wheels. Been occupied with another project, so haven't been able to dedicate much time to it past few weeks. I'm fine with that plan, and also plan to return to this bug when time allows.

@peternewman

Copy link
Copy Markdown
Member

Okay great. Just to confirm though, you didn't manage to reproduce it, or understand why it might have happened during your investigations @kecramer ?

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just one trivial change which I'll push in.

Comment thread olad/RDMHTTPModule.cpp
@peternewman peternewman self-assigned this Jan 16, 2019
@peternewman
peternewman merged commit a4d8717 into OpenLightingProject:master Jan 16, 2019
@peternewman peternewman mentioned this pull request Jul 12, 2026
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