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

Add PD after the initial configuration #205

Merged
merged 1 commit into from
Dec 1, 2024

Conversation

hahnnicholas
Copy link
Contributor

@hahnnicholas hahnnicholas commented Nov 13, 2024

Add the capability to add PD's in the CP after the initial initialization.
This can be used to add devices in run time.

@hahnnicholas hahnnicholas changed the title Add support to include CP after the initial configuration. Add PD after the initial configuration Nov 13, 2024
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.

After fixing the realloc() bug, and fixing the broken code formatting (see style guide), I will take a closer look at this PR.

src/osdp_cp.c Outdated Show resolved Hide resolved
@sidcha
Copy link
Member

sidcha commented Nov 27, 2024

Add the capability to add PD's in the CP after the initial initialization

Please add some motivation/problem-statement which lead you to make this change. PDs don't need to be added at runtime in most cases. Knowing why you are doing what you are doing, helps me better evaluate your change.

@hahnnicholas
Copy link
Contributor Author

Hi, thanks for your time.
It's not a issue but more a design related thing.
Sometimes applications are reader oriented rather then osdp PD oriented, so as in my case.
This implies that the application build the readers during initialization as they are configured in the database.
Every time a reader are built, the reader factory needs to check wich type this reader are and add their addrs to the CP to be managed.
In my case, it's easier to add pd's to the cp on the fly than have to wait for all the inicialization to full fill an array (that I don't know the size previously cause it are configured in the database) to only after that start the CP process.

src/osdp_cp.c Outdated Show resolved Hide resolved
@sidcha sidcha force-pushed the add-pd-after-setup branch 2 times, most recently from 7df7634 to 1d2868e Compare November 28, 2024 00:07
src/osdp_cp.c Outdated Show resolved Hide resolved
src/osdp_cp.c Outdated Show resolved Hide resolved
src/osdp_cp.c Outdated Show resolved Hide resolved
src/osdp_cp.c Outdated Show resolved Hide resolved
include/osdp.h Outdated Show resolved Hide resolved
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.

Posting these comments just for posterity. Most of these are nits, and since I was already reviewing them locally, just decided to address them locally to avoid a back-and-forth.

src/osdp_cp.c Outdated Show resolved Hide resolved
src/osdp_cp.c Outdated Show resolved Hide resolved
src/osdp_cp.c Outdated Show resolved Hide resolved
src/osdp_cp.c Outdated Show resolved Hide resolved
src/osdp_cp.c Outdated Show resolved Hide resolved
src/osdp_cp.c Outdated Show resolved Hide resolved
src/osdp_cp.c Outdated Show resolved Hide resolved
@sidcha sidcha force-pushed the add-pd-after-setup branch from bb9ebcd to acd2334 Compare November 30, 2024 10:03
Signed-off-by: Nicholas Hahn <[email protected]>
Signed-off-by: Siddharth Chandrasekaran <[email protected]>
@sidcha sidcha force-pushed the add-pd-after-setup branch from 0659ba6 to c41c77a Compare December 1, 2024 20:29
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.

Thanks for the PR.

@sidcha sidcha merged commit 15d70c3 into goToMain:master Dec 1, 2024
12 checks passed
@hahnnicholas
Copy link
Contributor Author

Thanks for your help, time and kindly review.

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