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

User-configurable buffer sizes #13

Open
dpkoch opened this issue Jan 21, 2021 · 5 comments
Open

User-configurable buffer sizes #13

dpkoch opened this issue Jan 21, 2021 · 5 comments

Comments

@dpkoch
Copy link
Owner

dpkoch commented Jan 21, 2021

Creating from comment by @RonaldEnsing on #1:

Why has the option to make buffer sizes configurable been removed later on [1]? I am working with UDP packets that exceed this size and I use async_comm as a library, which means I would need to modify the async_comm code in order to read the packets completely. I'd very much prefer if this is configurable and I can use the installed package without the need of cloning and modifying the syn_comm code.

@dpkoch
Copy link
Owner Author

dpkoch commented Jan 21, 2021

Hi @RonaldEnsing, thanks for reaching out!

The short answer is that I removed it because it didn't work. The longer answer is that the challenge is that the buffer size needs to be defined at compile time, since it's used to define some fixed-size arrays. So when async_comm (in its current form) is installed as a library, that buffer size has already been compiled in and can't be changed. As a result, the implementation I had would work when building async_comm from source in the subdirectory of your project, but not when linking against an installed version (it would look like it was configuring the buffer size, but those #defines didn't actually do anything at that point).

One solution here would be to have the various classes be templated on the buffer size. However, that would require converting (at least much of) the project to a header-only implementation. My reasoning was that rather than go that route, it would just require that those users (hopefully a small subset) who need to set custom header sizes would need to include async_comm as a submodule and modify it locally. I'm open to feedback on that though.

Perhaps a good compromise would be to put something like what I had before back in, which would allow those users to still include async_comm as a submodule, but at least be able to configure the buffer sizes from within their own project code, without modifying the async_comm code itself. I think I'd be happy to do that, I'd just want to figure out a way to avoid the previous problem where users who are linking against an installed package could do something that looks like they're configuring the buffer size, but in reality it does nothing.

@RonaldEnsing
Copy link

Thanks for the explanation. Is see your point. That makes sense. Other similar packages, like for example udp_com [1] have something similar, they simply set the buffer to such a large value that it is less likely to run into issues.

[1] https://github.com/continental/udp_com/blob/main/src/udp_com.cpp#L39

@RonaldEnsing
Copy link

The longer answer is that the challenge is that the buffer size needs to be defined at compile time, since it's used to define some fixed-size arrays.

Could you elaborate why these fixed size arrays are required at compile time?

@reinzor
Copy link
Contributor

reinzor commented May 11, 2021

Would it make sense to pass these parameters in the constructor so that these can be overloaded?

@reinzor
Copy link
Contributor

reinzor commented Jun 30, 2021

ping @dpkoch

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

No branches or pull requests

3 participants