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

New MCPWM APIs can't be parsed by SWIG (IDFGH-9046) #10449

Closed
surfskidude opened this issue Dec 28, 2022 · 5 comments
Closed

New MCPWM APIs can't be parsed by SWIG (IDFGH-9046) #10449

surfskidude opened this issue Dec 28, 2022 · 5 comments
Assignees
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally Type: Feature Request Feature request for IDF

Comments

@surfskidude
Copy link

Is your feature request related to a problem?

With version 4 I could use the SWIG binging generator (https://www.swig.org/) to create an interface that could be used by a scripting language. The new API is such a kludge with tons of macros and variable length arguments making it impossible to use a tool such as SWIG.

Describe the solution you'd like.

Please consider making an easier to use API suitable for SWIG.

Describe alternatives you've considered.

No response

Additional context.

No response

@surfskidude surfskidude added the Type: Feature Request Feature request for IDF label Dec 28, 2022
@suda-morris
Copy link
Collaborator

Hi @surfskidude Could you please give a concrete example of why the variable length arguments can't be parsed by the SWIG tool? BTW, macros like MCPWM_GEN_COMPARE_EVENT_ACTION are just helpers, not necessary.

@suda-morris suda-morris changed the title New MCPWM API in version 5 practically useless New MCPWM APIs can't be parsed by SWIG Dec 28, 2022
@igrr
Copy link
Member

igrr commented Dec 28, 2022

FWIW this issue also exists for bindings generators based on C++ templates. E.g. https://github.com/wasm3/wasm3/tree/main/platforms/cpp and https://github.com/ThePhD/sol2 would run into the same problem with variable length argument lists. The issue is due to the fact that these bindings generators rely on type information from the function signature. For varargs, there is no type information, so it's difficult to write a binder template which would determine the varargs type correctly.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Dec 28, 2022
@github-actions github-actions bot changed the title New MCPWM APIs can't be parsed by SWIG New MCPWM APIs can't be parsed by SWIG (IDFGH-9046) Dec 28, 2022
@espressif-bot espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Dec 28, 2022
@suda-morris
Copy link
Collaborator

Okay, thank you @igrr I see, the "fix" on the driver side is not hard but to provide a bunch of new functions to set the PWM generator's behavior one by one. e.g.:

mcpwm_generator_set_actions_on_compare_event --> mcpwm_generator_set_action_on_compare_event (thanks I was using plural in the varg version APIs)

However, I can see there are other public functions in the esp-idf that are also using the varargs (e.g. esp_eth_transmit_vargs). 🤔 Not sure if we're going to "fix" them.

@espressif-bot espressif-bot added Status: Opened Issue is new and removed Status: Selected for Development Issue is selected for development labels Dec 28, 2022
@surfskidude
Copy link
Author

I have not looked at all APIs, just the MCPWM API, which we use for servo control. The new MCPWM 5 API is much more complex and I do not think it is possible to use SWIG with this new API. We do not have the resources to hand-craft bindings. Will you continue supporting the 4.x release also in the future?

@suda-morris
Copy link
Collaborator

Will you continue supporting the 4.x release also in the future?

Yes, the 4.4 release is still in our service period. Check this.
BTW, the previous "legacy" driver APIs are still available even in esp-idf v5. You can use those APIs in driver/mcpwm.h.

The new MCPWM 5 API is much more complex

Right, but the complicity relies on the hardware itself. MCPWM is not a simple peripheral at all. The legacy driver gives us the illusion that it's simple because it made a lot of wrong assumptions to simplify the usage. In the new driver, we just exposed all the hardware abilities.

As I said in the above thread, we can provide non-vararg versions of APIs if you would like to. But we can't remove the existing vararg ones.

@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Dec 30, 2022
@espressif-bot espressif-bot added Status: Reviewing Issue is being reviewed Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: In Progress Work is in progress Status: Reviewing Issue is being reviewed Resolution: NA Issue resolution is unavailable labels Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally Type: Feature Request Feature request for IDF
Projects
None yet
Development

No branches or pull requests

4 participants