michallenc commented on issue #15567:
URL: https://github.com/apache/nuttx/issues/15567#issuecomment-3061671277
Hi @raiden00pl @ppisa @acassis. Looking at this after some time, the design
I would propose is following.
**File location**
- `drivers/can/can_bittiming.c` as a C file with needed functions
- as for header file, I am more inclined to put this in already existing
`include/nuttx/can/can_common.h` instead of creating a new separate header
**API**
Strongly inspired by RTEMS here, i would keep the function
`can_bitrate2bittiming`
```c
/* Controller's maximum and minimal possible constants */
struct can_bittiming_const {
const char *name;
uint32_t tseg1_min;
uint32_t tseg1_max;
uint32_t tseg2_min;
uint32_t tseg2_max;
uint32_t sjw_max;
uint32_t brp_min;
uint32_t brp_max;
uint32_t brp_inc;
};
/* Actual values or values to be set */
struct can_bittiming {
uint32_t bitrate;
uint32_t sample_point;
uint32_t tq;
uint32_t prop_seg;
uint32_t phase_seg1;
uint32_t phase_seg2;
uint32_t sjw;
uint32_t brp;
};
int can_bitrate2bittiming(uint32_t chip_frequency, struct can_bittiming *bt,
const struct can_bittiming_const *btc);
```
I am not sure about `nx_` prefix here. We don't use it anywhere in NuttX so
I would probably avoid bringing it here so we have unified API with other CAN
related functions (as `can_dlc2bytes` for example).
The tricky part here is how to set up bitrate, because we support both
SocketCAN and NuttX CAN. We have to keep `SIOCSCANBITRATE` for SocketCAN
compatibility, but this is fine since it gives you `can_ioctl_data_s` structure
with bitrate and sample point, so we can just pass these values to the common
bittiming function.
```C
struct can_ioctl_data_s
{
uint32_t arbi_bitrate; /* Classic CAN / Arbitration phase bitrate bit/s */
uint16_t arbi_samplep; /* Classic CAN / Arbitration phase input % */
uint32_t data_bitrate; /* Data phase bitrate bit/s */
uint16_t data_samplep; /* Data phase sample point % */
};
```
As for NuttX CAN, the ioctl `CANIOC_SET_BITTIMING` passes
`canioc_bittiming_s` structure. We would need it to pass `can_bittiming`
presented above instead.
```c
struct canioc_bittiming_s
{
#ifdef CONFIG_CAN_FD
uint8_t type; /* Nominal/Data bit timing. This is
* used to specify which bit timing
* should be set/obtained. Applies
* only if CAN FD is configured.
*/
#endif
uint32_t bt_baud; /* Bit rate = 1 / bit time */
uint8_t bt_tseg1; /* TSEG1 in time quanta */
uint8_t bt_tseg2; /* TSEG2 in time quanta */
uint8_t bt_sjw; /* Synchronization Jump Width in
time quanta */
};
```
I would keep this ioctl for backwards compatibility (mark is as deprecated
and keep it few LTS releases) and add new ioctl `CANIOC_SET_BITRATE` that would
pass `canioc_set_bittiming_s`.
```c
struct canioc_set_bittiming_s {
/* nominal or data */
uint16_t type;
/* from bitrate or from given bittiming */
uint16_t from;
struct can_bittiming bittiming;
};
```
This is the same approach we used in RTEMS. If user wants to set bittiming
from required bitrate, he just fills the `bitrate` field of `can_bittiming` and
the driver does all calculation for him. But sometimes it is required to
precisely set phase segments and so on, therefore it is also possible to fill
`can_bittiming` structure by itself and use the same ioctl to set bittiming
(different `from` field value).
Similarly, we would have an ioctl to obtain the current bittiming. It would
return the following structure. I am not sure about the name of the ioctl yet,
`CANIOC_GET_BITTIMING` would be ideal, but this is already used by the old
ioctl call.
```c
struct canioc_get_bittiming_s {
uint16_t type;can_bittiming
struct can_bittiming bittiming;
struct can_bittiming_const bittiming_const;
};
```
Let me know what you think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
