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

Make some internal methods accessible publicly #238

Open
jay-sridharan opened this issue Mar 8, 2024 · 6 comments
Open

Make some internal methods accessible publicly #238

jay-sridharan opened this issue Mar 8, 2024 · 6 comments

Comments

@jay-sridharan
Copy link

jay-sridharan commented Mar 8, 2024

Hello again!

I am using this library for a real-time motion control application. To this effect, I cannot have any syscalls once the system is operational, as it introduces non-determinism. This includes malloc.

Tinyspline is really close to what I need because memory allocation is well-contained, and I have been able to write most of what I need using pre-allocated buffers. However, there are some things that would be useful to have exposed to make this possible.

What are your thoughts on including these definitions in tinyspline.h ?

    struct tsBSplineImpl
    {
        size_t deg;
        size_t dim;
        size_t n_ctrlp;
        size_t n_knots;
    };

    struct tsDeBoorNetImpl
    {
        double u;
        size_t k;
        size_t s;
        size_t h;
        size_t dim;
        size_t n_points;
    };
    
    tsError ts_int_bspline_eval_woa(const tsBSpline *spline, tsReal u, tsDeBoorNet *net, tsStatus *status);

I need ts_int_bspline_eval_woa because ts_bspline_eval calls malloc. The struct definitions are useful to have so that I can call sizeof(tsBSplineImpl) in the calculation for my static memory allocation, and similarly for tsDeBoorNetImpl

@msteinbeck
Copy link
Owner

Hi @jay-sridharan,

TinySpline is designed in such a way for a purpose. By "hiding" the internals of the structs, all function can rely on data integrity. However, I see your issue, so let's try to solve it.

  • You could declare the internal elements as extern in your code.
  • You could include tinyspline.c. This gives you access to all elements of the library.
  • We could provide a preprocessor definition, e.g., TINYSPLINE_EXPORT_INTERNALS, which makes the hidden elements visible to the client.

Regarding the preprocessor definition, feel free to make a pull request.

@jay-sridharan
Copy link
Author

jay-sridharan commented Mar 11, 2024

Thanks for the response, Marcel!

I tried option #1, however because the default visibility is set to hidden, the compiled library does not actually make these symbols accessible.

CXX_VISIBILITY_PRESET hidden

The TINYSPLINE_SHARED_EXPORT flag makes the "external" facing methods export a symbol in the library.

#define TINYSPLINE_SHARED_EXPORT __attribute__ ((visibility ("default")))

I could of course compile the library from source, but ideally I want to upstream a way for this to be possible just from having the packaged release installed on the system.

What do you think of this idea? We can

  • change the default visibility such that all internal functions are part of the symbol table (this will allow me to extern the internal methods in my own code)
  • Add two extern variables to tinyspline.h for the sizes of the implementation structs. The structs themselves will be defined in the implementation, and the size variables will be declared there, but they will be accessible.
// tinyspline.h
extern size_t SIZEOF_BSPLINE_IMPL;
extern size_t SIZEOF_DEBOORNET_IMPL;

// tinyspline.c
size_t SIZEOF_BSPLINE_IMPL = sizeof(tsBSplineImpl);
size_t SIZEOF_DEBOORNET_IMPL = sizeof(tsDeBoorNetImpl);
  • Lastly, add an extern variable for the version. This way, if I use any internal methods, I can static_assert that the version of the library being used is what I wrote the code for. In the future, if someone were to change how the internal methods are accessed, they will be forced to change the assertion that checks the tinyspline version.
// tinyspline.h
extern int TS_VERSION_MAJOR;
extern int TS_VERSION_MINOR;
extern int TS_VERSION_PATCH;

// tinyspline.c
int TS_VERSION_MAJOR = 0;
int TS_VERSION_MINOR = 7;
int TS_VERSION_PATCH = 0;

@jay-sridharan
Copy link
Author

Hi @msteinbeck , just wanted to follow up on this! If this approach seems reasonable, I'm happy to put in a pull request!

@msteinbeck
Copy link
Owner

Hi @jay-sridharan,

If this approach seems reasonable, I'm happy to put in a pull request!

This seems indeed reasonable. Feel free to create a pull request :). Please keep in mind to add prefixes to the variable names. For instance, TS_SIZEOF_BSPLINE_IMP.

@jay-sridharan
Copy link
Author

jay-sridharan commented Mar 22, 2024

Hi @msteinbeck

I gave this method a shot, but ran into some issues....
Because the memory allocation I am doing is static, in an object declaration, I need to have the size information as part of my header file. This doesn't work with what I had suggested earlier because the size is not known at compile-time, only at link-time.

I put in #240 which tries a different approach of adding another header file which declares internal methods. This file is added to the release package, but the header file throws a warning when included to alert the user that they probably don't want to be including it.

Let me know what you think!

@jay-sridharan
Copy link
Author

I figured this is easier on the client code than a TINYSPLINE_EXPORT_INTERNALS macro, and doesn't change much about tinyspline.
This way, definitions don't need to be copied from the tinyspline source code and also does not require the client to build the library from source.

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

2 participants