-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Implemented personality and personality_description parameters #92
Conversation
701652e
to
c8c7c54
Compare
c8c7c54
to
9ed5b01
Compare
Thanks for the hard work on this and #93. I haven't looked closely at these, but if they are working for you then it is likely that this PR is at least 80% complete. There are a few minor changes that will need to be made but I will share those with you when I can review this PR more closely! I think it would be helpful if I documented some core concepts of the library better so I think I'll add that to my to-do list! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work again! Thanks for bearing with my comments. :D
2456279
to
15a7d9e
Compare
I was commenting about getters/setters in #93 and it made me realize I forgot to address getters/setters in this PR. For Because DMX start address can be used in a DMX context as well as an RDM context I thought it would be confusing for the user to have to remember that only one set of getter/setter functions existed. I don't think this is an anti-pattern, but I would love some input if I am mistaken. Should RDM getter/setter wrapper functions also be implemented for the PIDs in this PR? |
Having two methods that do the same thing can also be confusing. In the end either way is fine as long as it is consistent imo.
Since the personality is useful in both contexts, yes. |
Merged. Thank you for the great work! |
This is my first attempt at personality and personality_description getters/setters.
It is implemented on top of PR #90.
I tested it using my rdm tester and it works.
As with the other PR I am not sure if this is the correct way to implement it.
I would appreciate it if you review the code and point out anything odd :-)
The PR is kinda hard to read because it contains all the changes from #90.
Reading the diff against #90 is much easier.
(or just read the last commit, same thing :D)