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

Python Bindings: CP add CMD_STATUS support #116

Closed
wants to merge 1 commit into from

Conversation

Stuckya
Copy link
Contributor

@Stuckya Stuckya commented May 19, 2023

I ran into an incompatibility when trying to use some new functionality in the Python bindings: #105

Currently osdp.CMD_STATUS is not exposed to the user of the Python bindings.

Additionally, the evaluated enum value is discarded by pyosdp_make_struct_cmd_dummy.

@Stuckya Stuckya mentioned this pull request May 19, 2023
@@ -363,7 +363,11 @@ static int pyosdp_make_struct_cmd_file_tx(struct osdp_cmd *p, PyObject *dict)
}

/* Dummies for commands that don't have any body */
static int pyosdp_make_struct_cmd_dummy(struct osdp_cmd *cmd, PyObject *obj) { return 0; }
static int pyosdp_make_struct_cmd_dummy(struct osdp_cmd *cmd, PyObject *obj) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This appears to be the only usage of this function. Should we rename the function and update the comment since it has a body now?

Copy link
Member

@sidcha sidcha Jun 13, 2023

Choose a reason for hiding this comment

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

No, the issue is because we are setting cmd->id in the wrong place. The caller pyosdp_make_struct_cmd should set it so the dummy can stay as it is now.

@sidcha
Copy link
Member

sidcha commented Jun 13, 2023

@Stuckya You can fix this if you are up for it. If not, I will close this PR and fix it myself in a couple of days.

@sidcha sidcha closed this in 4feb2f8 Jun 27, 2023
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.

None yet

2 participants