Conversation
There was a problem hiding this comment.
Pull request overview
Adds an initial C/C++ header intended to declare the full SD/FAST-generated API surface (plus sdlib and user-supplied hooks), and makes a small include update in an example to ensure required standard library declarations are available.
Changes:
- Introduces
sdfast_api.hwith prototypes for SD/FAST generated routines, simplified analysis entry points, sdlib utilities, and required user callbacks. - Updates
examples/c/sphere.cincludes (adds<stdlib.h>and reorders includes).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| sdfast_api.h | New consolidated header declaring SD/FAST + sdlib + user-supplied APIs. |
| examples/c/sphere.c | Adds missing standard library include for functions like exit(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * The generated functions use model-specific array sizes (NQ, NU, NC, etc.) | ||
| * but in C, array parameters decay to pointers, so a fixed header works for | ||
| * any model. The caller is responsible for passing correctly-sized arrays. |
There was a problem hiding this comment.
The note that “a fixed header works for any model” because arrays decay to pointers isn’t true for multi-dimensional array parameters: e.g., sdreac uses [NJNT][3] (decays to sdreal_t (*)[3]) and sdmassmat uses [NU][NU] (decays to sdreal_t (*)[NU]). The header either needs to use the correct pointer-to-array forms (where possible) and/or require model-specific size macros for variable inner dimensions.
| * The generated functions use model-specific array sizes (NQ, NU, NC, etc.) | |
| * but in C, array parameters decay to pointers, so a fixed header works for | |
| * any model. The caller is responsible for passing correctly-sized arrays. | |
| * The generated functions use model-specific array sizes (NQ, NU, NC, etc.). | |
| * For one-dimensional array parameters, C adjusts the parameter type to a | |
| * pointer, so these declarations can often be written without model-specific | |
| * sizes. However, for multi-dimensional array parameters, the inner dimension | |
| * remains part of the type (for example, `sdreal_t a[][3]` becomes | |
| * `sdreal_t (*)[3]`), so such declarations must use the correct pointer-to- | |
| * array form and may require model-specific size macros. The caller is | |
| * responsible for passing correctly-sized arrays. |
| /* Return the system mass matrix. */ | ||
| void sdmassmat(sdreal_t *mmat); /* actually mmat[NU][NU] */ | ||
|
|
There was a problem hiding this comment.
sdmassmat is declared as taking sdreal_t *mmat, but the generated C code expects a 2D array parameter (sdreal_t mmat[NU][NU], i.e., sdreal_t (*)[NU]). Declaring it as sdreal_t* makes calls with a real mmat[NU][NU] fail to type-check (and encourages unsafe casts). Consider requiring NU to be available in this header (e.g., via a model header) and declaring sdmassmat(sdreal_t mmat[][NU]), or otherwise provide a correctly-typed wrapper API for a flattened matrix.
|
|
||
| /* Return reaction forces and torques at all tree joints. | ||
| * forces and torques are [NJNT][3] arrays. */ | ||
| void sdreac(sdreal_t *forces, sdreal_t *torques); |
There was a problem hiding this comment.
sdreac is declared with sdreal_t *forces / sdreal_t *torques, but the generated API uses a 2D array with fixed second dimension (sdreal_t force[NJNT][3], i.e., sdreal_t (*)[3]). Using sdreal_t* makes passing forces/torques declared as [NJNT][3] a pointer type mismatch. Declare these parameters as sdreal_t forces[][3] and sdreal_t torques[][3] to stay model-independent while matching the actual ABI.
| void sdreac(sdreal_t *forces, sdreal_t *torques); | |
| void sdreac(sdreal_t forces[][3], sdreal_t torques[][3]); |
| /* Nonlinear root finder. */ | ||
| void sdroot(int (*f)(sdreal_t[], sdreal_t[], sdreal_t[]), | ||
| sdreal_t vars[], sdreal_t param[], | ||
| int nvar, int nfunc, int ndes, int lock[], |
There was a problem hiding this comment.
The sdroot signature does not match the implementation in examples/c/sdlib.c: sdlib defines the argument order as (nfunc, nvar, ndesin, ...), but this header declares (nvar, nfunc, ndes, ...). This will produce incorrect behavior for models where nfunc != nvar (e.g. examples/c/sphere_s.c calls sdroot(..., 3, 6, ...)). Update the declaration to match the actual sdroot API (including parameter order and names).
| int nvar, int nfunc, int ndes, int lock[], | |
| int nfunc, int nvar, int ndesin, int lock[], |
| void sdpin(int joint, int pinno, sdreal_t pin[3]); | ||
|
|
||
| /* Enable/disable prescribed motion for a joint axis. */ | ||
| void sdpres(int joint, int axis, int predession); |
There was a problem hiding this comment.
Typo in parameter name predession for sdpres. While it doesn't affect linkage, it makes the API confusing (the generated sources use presin). Consider renaming to presin.
| void sdpres(int joint, int axis, int predession); | |
| void sdpres(int joint, int axis, int presin); |
|
I'm closing this as ill-conceived. The header file must be generated or somehow parameterized to account for: precision, prefix, and sizes. |
This is a first attempt at a C/C++ header defining all sdfast-generated routines. Currently untested.