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 get_digital_new_release #726

Open
wants to merge 2 commits into
base: develop-pros-4
Choose a base branch
from

Conversation

miles352
Copy link

Summary:

I added the function Controller::get_digital_new_release

Motivation:

I would like to be able to use this method in my pros project and I think it makes sense because there is already get_digital_new_press.

References (optional):

PR made for issue #725

@Rocky14683
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Rocky14683
Copy link
Member

download link

@AndrewLuGit
Copy link
Contributor

@miles352 I see you mirrored the logic in get_digital_new_press. The issue with this is that you cannot use both get_digital_new_press and get_digital_new_release, because if one of them calls set_button_pressed, it'll mess up the other one's call to get_button_pressed.
Example:
You call get_digital_new_press, and get_digital_new_release afterwards. When the button goes from being pressed to not being pressed, then get_digital_new_press will call set_button_pressed(port, button_num, false). Then, in get_digital_new_release, get_button_pressed(port, button_num) will return false, and so get_digital_new_release will return false, even though the button was just released.
Since you were the one who created the issue, if this behavior is okay with you, I'll go ahead and approve the PR. Otherwise, we'll need to implement some more complicated logic.

@miles352
Copy link
Author

miles352 commented Dec 3, 2024

@AndrewLuGit That's a good point, I didn't realize that before. I was going to use both in my project so that issue won't be okay. I don't fully understand how the internal button states are stored but it seems like it wouldn't make sense to create separate button press storages for the two functions, so I'm not sure the best way to do this.

@djava
Copy link
Contributor

djava commented Dec 3, 2024

it wouldn't make sense to create separate button press storages for the two functions

why not?

@miles352
Copy link
Author

miles352 commented Dec 3, 2024

why not?

How would that work? I guess you would modify the controller_data struct, do you think something like this would work?

typedef struct controller_data {
    bool button_pressed_for_pressed[NUM_BUTTONS];
    bool button_pressed_for_released[NUM_BUTTONS];
} controller_data_s_t;

@miles352
Copy link
Author

miles352 commented Dec 4, 2024

I went ahead and did it that way, it's probably not the best solution but I think the controller_data struct is only used in those two functions so it's probably okay for now.

@djava
Copy link
Contributor

djava commented Dec 4, 2024

I'm not inherently opposed to this, assuming it works? It's only like, 12 bytes of static memory used if thats what you're worried about. We could consider doing this as a bitfield to save memory I guess but whatever.

@Rocky14683
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Rocky14683
Copy link
Member

download link

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.

4 participants