drivers: tmc5240: Added wrapper for TMC5240 from TMC-API.#2914
drivers: tmc5240: Added wrapper for TMC5240 from TMC-API.#2914kister-jimenez wants to merge 5 commits intomainfrom
Conversation
6c58e6d to
c0cdea9
Compare
| * | ||
| * @return 1 (success) | ||
| */ | ||
| uint8_t reset(void) |
There was a problem hiding this comment.
can you make these function names less generic?
There was a problem hiding this comment.
This is the actual name of the function that TMC-API expects for the reset.
|
|
||
| switch (channel->type) { | ||
| case IIO_ACCEL: | ||
| ret = tmc5240_set_amax(iio_tmc5240->tmc5240_dev, val); |
There was a problem hiding this comment.
shouldn't we have a setter function for get_current_accel?
There was a problem hiding this comment.
There's no setter for current/actual accel since we can't really control the acceleration. We can only set the max.
There was a problem hiding this comment.
Indeed, there is no control by the user over the actual real-time acceleration, since that is taken care of by the controller IC itself, being one if its main purposes.
However, since we are talking about acceleration I see that this IC has support for A1, which would mean the user could have more control over the ramp used for acceleration. In some applications this can be very useful, and sometimes a key aspect to why stepper motors are used.
It would be nice to have support for that as well (this would mean D1 would have support also).
There was a problem hiding this comment.
Was not planning to add A1/D1 as the application I'm planning to use this driver only requires the bare minimum configurations for velocity and position control. But I can add these in the init_params and some debug ABI to configure the these.
tmc5240 is just a wrapper around TMC-API, which is a third-party library. We don't want to include it in driver build checks, but there will be a project that will be using this that is currently being drafted. Signed-off-by: Kister Genesis Jimenez <kister.jimenez@analog.com>
Adding the TMC-API as a submodule so that drivers for Trinamic motor controllers will only need to wrap the API instead of re-implementing functionality from scratch. This will make it easier to maintain the drivers and keep them up to date with the latest features and bug fixes from Trinamic. Signed-off-by: Kister Genesis Jimenez <kister.jimenez@analog.com>
Signed-off-by: Kister Genesis Jimenez <kister.jimenez@analog.com>
c0cdea9 to
5f73416
Compare
|
V2:
|
| ret = no_os_spi_write_and_read(dev->spi_desc, data, dataLength); | ||
| if (ret) { | ||
| /* TMC-API doesn't support error return, so we can only log this */ | ||
| pr_err("Failed to read/write SPI for TMC5240 IC: %d\r\n", ret); |
There was a problem hiding this comment.
This can be a problem to be fair. If no error code is returned and is instead only logged as a print in the terminal it may harm the final application where this driver will be used.
e.g. the driver is used with a stepper that should change speed/position fast, and the communication (SPI in this case) fails, the value of the speed/position could remain unchanged and therefore produce faulty behavior, perhaps even instability if something else depends on the stepper's actions. In this case an error code returned is helpful since if the communication breaks, the motor could be stopped (by toggling a reset/power pin, or by switching off the gates and sometimes the power supply itself). With no error handling, automation of the process becomes harder, requiring otherwise unnecessary workarounds.
There was a problem hiding this comment.
Should I just implement a no-OS native driver in this case? This function here is used by TMC-API so the function signature has to match. Converting this to a no-OS driver is trivial but I was just trying to avoid having 2 baremetal drivers for TMC5240 in ADI repo.
There was a problem hiding this comment.
This depends. For example, TMC7300 already has a native no-OS driver and a TMC-API driver. On the other hand it would be nice to not have duplicate code on the repos, but parts of the TMC-API seem to either not address errors at all, or even use to generic API names, that can be integrated harder in a wider application to be fair. In this case my opinion is that a native driver would be better and perhaps it could be more compact and efficient, though another opinion on this matter would help.
Pull Request Description
This PR introduces a tmc5240 driver with IIO support wrapping the TMC-API. TMC-API is added as submodule and built as library. This is going to be used by at least 2 projects. There is a draft PR for ADMT4000 calibration kit that is dependent on this PR. I decided to split the PR to make it easier for review and so that the other projects dependent on this driver can use the driver without waiting for the ADMT4000.
PR Type
PR Checklist