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

Implemented personality and personality_description parameters #92

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

arneboe
Copy link
Contributor

@arneboe arneboe commented Aug 20, 2023

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)

@someweisguy
Copy link
Owner

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!

@arneboe arneboe changed the title WIP: Implemented personality and personality_description parameters Implemented personality and personality_description parameters Aug 23, 2023
Copy link
Owner

@someweisguy someweisguy left a 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

src/rdm/responder.c Outdated Show resolved Hide resolved
src/dmx/hal.c Show resolved Hide resolved
@someweisguy
Copy link
Owner

someweisguy commented Aug 25, 2023

I was commenting about getters/setters in #93 and it made me realize I forgot to address getters/setters in this PR.

For RDM_PID_DMX_START_ADDRESS, there are two getters and two setters. The getters are dmx_get_start_address() and rdm_get_dmx_start_address(). And there is a similar prefix change for the setter: dmx_set_start_address() and rdm_set_dmx_start_address(). I implemented dmx_get_start_address() and its counterpart, and made rdm_get_dmx_start_address() a wrapper function. I also made a wrapper for the setter.

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?

Edit: Here's an example wrapper function.

@arneboe
Copy link
Contributor Author

arneboe commented Aug 25, 2023

For RDM_PID_DMX_START_ADDRESS, there are two getters and two setters. The getters are dmx_get_start_address() and rdm_get_dmx_start_address(). And there is a similar prefix change for the setter: dmx_set_start_address() and rdm_set_dmx_start_address(). I implemented dmx_get_start_address() and its counterpart, and made rdm_get_dmx_start_address() a wrapper function. I also made a wrapper for the setter.

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.

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.

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?

Since the personality is useful in both contexts, yes.

@someweisguy someweisguy merged commit 251f99f into someweisguy:release/v3.1 Sep 6, 2023
@someweisguy
Copy link
Owner

Merged. Thank you for the great work!

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