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

Increasing the max data contained in an OSDP event to 128bytes #152

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Increasing the max data contained in an OSDP event to 128bytes #152

merged 1 commit into from
Dec 18, 2023

Conversation

dmercer-google
Copy link
Contributor

This is necessary because we are dealing with a Manufacturer Specific Response that contains at least 73 bytes of date. The data is an ASN.1 serialized ECDSA P256 signature. In the case of larger curves (e.g. P384) the signature will be longer but 128 bytes will handle it fine. In the case of RSA signatures it is almost for sure that 128bytes will be too small but that is not a use case we are concerned with at the moment nor is it one we can test.

Copy link
Member

@sidcha sidcha left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@sidcha sidcha merged commit 7e2c92b into goToMain:master Dec 18, 2023
10 checks passed
@Sympatron
Copy link
Contributor

Sympatron commented Dec 19, 2023

Won't this greatly increase memory consumption? If so, I don't think this would be a good default then.
I did not check yet, but I would expect the RAM usage to increase by 2KB with OSDP_CP_CMD_POOL_SIZE=32.

@sidcha
Copy link
Member

sidcha commented Dec 19, 2023

@Sympatron, I was aware of this when I merged the change. I wasn't worried because that whole issue is due to a relic design decision of having internal and external command use the same structure. I plan on spending some time to decouple those two and then OSDP_CP_CMD_POOL_SIZE will be exclusively meant for internal commands which is at most a few bytes each.

sidcha added a commit that referenced this pull request Jan 6, 2024
Since we bumped the size of event data len to 128 in #152, the
command/event pool size of 32 uses almost 4KiB of RAM. To fix this CP
state machine was completely rewritten to not use the command queue
entirely. With this change, the command/event queue is exclusively meant
for commands and events sent from the application to libosdp and does
not need to be as large as 32.

Reduce this queue size to 4 -- thereby reducing the total memory used to
just 512 bytes.

Signed-off-by: Siddharth Chandrasekaran <[email protected]>
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.

3 participants