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.

Sign up for GitHub

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

Add support for the STM32Gxx chipset and add CANFD functionality to the driver #126

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Add support for the STM32Gxx chipset and add CANFD functionality to the driver #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ace1dcf
Change the can_data_t to use the CAN_HandleTypedef
ryedwards Nov 16, 2022
005e0ae
Add the base code for CANFD
ryedwards Nov 16, 2022
b529ef8
Add the USB defines for the SMTG0
ryedwards Nov 16, 2022
ed2982b
Change budgetcan channels back to default and fix sleep pin
ryedwards Nov 16, 2022
052bdec
Fix the standby pin set/reset to use the active high/low define
ryedwards Nov 16, 2022
6cd5fb4
Change gs_host_frame to a compiler directive
ryedwards Nov 16, 2022
6e5b893
Add timestamp back into the CANFD frame
ryedwards Nov 16, 2022
a0b1d0f
Add minimal changes to get ip link "canfd on" to execute
ryedwards Nov 16, 2022
25bc304
Uncrustify
ryedwards Nov 16, 2022
a809d4c
Changes to fully support CANFD frames on the bus
ryedwards Nov 16, 2022
9ad9177
Uncrustify
ryedwards Nov 16, 2022
6682ebf
Merge branch 'candle-usb:master' into implement-CANFD
ryedwards Nov 16, 2022
d7bace8
Removed unneeded rx and tx can buffers
ryedwards Nov 17, 2022
1efa7eb
Apply recommended change to storage of bit timing values
ryedwards Nov 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions include/can.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,24 +33,37 @@ THE SOFTWARE.

#include "gs_usb.h"

#if defined(FDCAN1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is probably best not to use FDCAN1 directly, but abstract it to STM_FDCAN_SUPPORT or something similar.

Copy link
Collaborator

@marckleinebudde marckleinebudde Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've renamed this CONFIG_CANFD in my 97e4b1a#diff-f20ab3eb0041360bbad27bdb1173ada9c2818c430536c89ccd514d822c43a1ecL240 branch.

We decided to not take this PR, but to clean it up and add multichannel support first. The multichannel work is happening in #139. It already contains cleanups for CAN-FD support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this to match many other implementation that use the HAL. Making an extra define adds an extra step. I used FDCAN1 in all areas where the HARDWARE needs to be configured (specific to the differences between the bxCAN and FDCAN registers). In another codebase I developed, though, I used "CANFD_SUPPORT" in areas that are non-HW dependent to turn on and off the USB driver level FD features since FDCAN can also run in classic mode if, lets say, a board was developed that is using an FDCAN uC but does not have the xcvr or desire to "report" CANFD support.

#define GS_HOST_FRAME gs_host_frame_canfd
#define GS_HOST_FRAME_CLASSIC gs_host_frame
#else
#define GS_HOST_FRAME gs_host_frame
#endif

typedef struct {
CAN_TypeDef *instance;
uint16_t brp;
uint8_t phase_seg1;
uint8_t phase_seg2;
uint8_t sjw;
#if defined(FDCAN1)
FDCAN_HandleTypeDef channel;
#else
CAN_HandleTypeDef channel;
#endif
} can_data_t;

#if defined(FDCAN1)
void can_init(can_data_t *hcan, FDCAN_GlobalTypeDef *instance);
void can_enable(can_data_t *hcan, bool loop_back, bool listen_only, bool one_shot, bool can_mode_fd);
bool can_set_data_bittiming(can_data_t *hcan, uint16_t brp, uint8_t phase_seg1, uint8_t phase_seg2, uint8_t sjw);
#else
void can_init(can_data_t *hcan, CAN_TypeDef *instance);
bool can_set_bittiming(can_data_t *hcan, uint16_t brp, uint8_t phase_seg1, uint8_t phase_seg2, uint8_t sjw);
void can_enable(can_data_t *hcan, bool loop_back, bool listen_only, bool one_shot);
#endif
bool can_set_bittiming(can_data_t *hcan, uint16_t brp, uint8_t phase_seg1, uint8_t phase_seg2, uint8_t sjw);
void can_disable(can_data_t *hcan);
bool can_is_enabled(can_data_t *hcan);

bool can_receive(can_data_t *hcan, struct gs_host_frame *rx_frame);
bool can_receive(can_data_t *hcan, struct GS_HOST_FRAME *rx_frame);
bool can_is_rx_pending(can_data_t *hcan);

bool can_send(can_data_t *hcan, struct gs_host_frame *frame);
bool can_send(can_data_t *hcan, struct GS_HOST_FRAME *frame);

/** return CAN->ESR register which contains tx/rx error counters and
* LEC (last error code).
Expand All @@ -61,4 +74,4 @@ uint32_t can_get_error_status(can_data_t *hcan);
* @param frame : will hold the generated error frame
* @return 1 when status changes (if any) need a new error frame sent
*/
bool can_parse_error_status(uint32_t err, uint32_t last_err, can_data_t *hcan, struct gs_host_frame *frame);
bool can_parse_error_status(uint32_t err, uint32_t last_err, can_data_t *hcan, struct GS_HOST_FRAME *frame);
1 change: 0 additions & 1 deletion include/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,6 @@ THE SOFTWARE.
#define CAN_INTERFACE FDCAN1
#define CAN_INTERFACE2 FDCAN2
#define CAN_CLOCK_SPEED 64000000
#define NUM_CAN_CHANNEL 2
#define CANFD_SUPPORT

#define nCANSTBY_Port GPIOA
Expand Down
3 changes: 3 additions & 0 deletions include/gs_usb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,4 +311,7 @@ struct gs_host_frame_canfd {
u8 reserved;

u8 data[64];

u32 timestamp_us;

} __packed __aligned(4);
10 changes: 9 additions & 1 deletion include/usbd_gs_can.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,13 @@ THE SOFTWARE.

/* Define these here so they can be referenced in other files */

#if defined(FDCAN1)
#define CAN_DATA_MAX_PACKET_SIZE 64 /* Endpoint IN & OUT Packet size */
#define CAN_CMD_PACKET_SIZE 72 /* Control Endpoint Packet size */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this works? AFAIK USB 2 packets are limited to 64 bytes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK - This is wrong. Full Speed USB frames, this is 11 Mbit/s, that all devices use for now, is limited to 64 bytes. Some of the STM µC support USB high speed (480 Mbit/s) with an external PHY, but that's not supported for now.

Copy link
Collaborator

@marckleinebudde marckleinebudde Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed this in my branch:
https://github.com/candle-usb/candleLight_fw/pull/139/files#diff-fef78b744980788a032c14aaedd228baf163f17cd2b96739c6040938421cad1cR42

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No...you are confusing bulk packet size with message packet size. The labeling of these parameters with "packet" may be confusing. Data payloads are automatically "chunked" by the USB stack. Since the control message to handle the data bitrate config (USBD_GS_CAN_btconst_extended) is larger than 64 bytes it is technically split into two "frames". The stack automatically takes care of this and the callback receives a 72 byte message. If this is not 72 then the "ep0_buf" will overflow in the "GS_USB_BREQ_DATA_BITTIMING" case in the "USBD_GS_CAN_EP0_RxReady" function and VERY bad things happen!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've learned though other USB projects to not assume anything based on the "64 byte" rule. Many stack implementations do not follow the USB 2.0 spec perfectly and you will run into buffer overruns or missed packets very easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marckleinebudde I checked your branch and it is the correct method for implementation (as usual) :)

Copy link
Collaborator

@marckleinebudde marckleinebudde Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid confusion I've renamed CAN_CMD_PACKET_SIZE to GS_CAN_EP0_BUF_SIZE.

Since the control message to handle the data bitrate config (USBD_GS_CAN_btconst_extended) is larger than 64 bytes it is technically split into two "frames". The stack automatically takes care of this and the callback receives a 72 byte message.

The GS_USB_BREQ_BT_CONST_EXT is Device -> Host, but mainline copies it to hcan->ep0_buf, I've fixed this in my branch. Device -> Host control messages are send directly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marckleinebudde I checked your branch and it is the correct method for implementation (as usual) :)

It's still untested :)

#else
#define CAN_DATA_MAX_PACKET_SIZE 32 /* Endpoint IN & OUT Packet size */
#define CAN_CMD_PACKET_SIZE 64 /* Control Endpoint Packet size */
#endif
#define USB_CAN_CONFIG_DESC_SIZ 50
#define NUM_CAN_CHANNEL 1
#define USBD_GS_CAN_VENDOR_CODE 0x20
Expand All @@ -58,6 +63,9 @@ extern USBD_ClassTypeDef USBD_GS_CAN;
// RX FIFO size chosen according to reference manual RM0368 which suggests
// using (largest packet size / 4) + 1
# define USB_RX_FIFO_SIZE ((256U / 4U) + 1U)
#elif defined(STM32G0)
# define USB_INTERFACE USB_DRD_FS
# define USB_INTERRUPT USB_UCPD1_2_IRQn
#endif

uint8_t USBD_GS_CAN_Init(USBD_HandleTypeDef *pdev, queue_t *q_frame_pool, queue_t *q_from_host, led_data_t *leds);
Expand All @@ -70,5 +78,5 @@ bool USBD_GS_CAN_CustomDeviceRequest(USBD_HandleTypeDef *pdev, USBD_SetupReqType
bool USBD_GS_CAN_CustomInterfaceRequest(USBD_HandleTypeDef *pdev, USBD_SetupReqTypedef *req);

bool USBD_GS_CAN_DfuDetachRequested(USBD_HandleTypeDef *pdev);
uint8_t USBD_GS_CAN_SendFrame(USBD_HandleTypeDef *pdev, struct gs_host_frame *frame);
uint8_t USBD_GS_CAN_SendFrame(USBD_HandleTypeDef *pdev, struct GS_HOST_FRAME *frame);
uint8_t USBD_GS_CAN_Transmit(USBD_HandleTypeDef *pdev, uint8_t *buf, uint16_t len);
Loading