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

Can Bootloader #38

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Can Bootloader #38

wants to merge 7 commits into from

Conversation

Roshyoshi
Copy link

No description provided.

Roshyoshi and others added 4 commits February 11, 2025 23:45
FW-333 Port CAN Bootloader to MS16

Enabled CAN bootloader using bootcan settings, following similar process to can_hw.c. Unsure of whether to start, and rx/tx queues and filters
Transmit and Receive CAN bootloader using bootcan settings, following process in can_hw.c. Unsure of what data in HAL_CAN_Message to use for receive function.
Copy link
Member

@Akashem06 Akashem06 left a comment

Choose a reason for hiding this comment

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

Lgtm, a few small changes and then ill merge it

@@ -37,6 +40,13 @@
* Higher bitrates require shorter time quantas
* Bitrate selection impacts signal reliability and bus length
*/

typedef struct BootCanTiming {
Copy link
Member

Choose a reason for hiding this comment

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

Remove line 43. Also can you follow this syntax instead (its the same thing, its just so our entire codebase adheres to 1 standard)

typedef struct {

} BootCanTiming;

You don't need to BootCanTiming after typedef struct, only needed at the end

@@ -45,8 +55,17 @@ typedef enum {
NUM_BOOT_CAN_BITRATES /**< Number of supported bit rates */
} Boot_CanBitrate;

static BootCanTiming s_timing[NUM_BOOT_CAN_BITRATES] = {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to the .c file?

@@ -107,4 +126,6 @@ BootloaderError boot_can_transmit(uint32_t id, bool extended, const uint8_t *dat
*/
BootloaderError boot_can_receive(Boot_CanMessage *msg);

#
Copy link
Member

Choose a reason for hiding this comment

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

Remove #

#define MAX_TX_MS_TIMEOUT 20

static CAN_HandleTypeDef hcan;
static CAN_HandleTypeDef *hcanLoc = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Whats this used for

Copy link
Member

Choose a reason for hiding this comment

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

Oh i see u reasign hcan to hcanLoc. Why not just use one instance?, we don't need to have two variables for the pointer and the instance.

Copy link
Member

Choose a reason for hiding this comment

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

On this note can u rename this to s_can_handle, just so it adheres to our other apis

hcan.Init.Mode = can_mode;
hcan.Init.SyncJumpWidth = CAN_SJW_1TQ;
hcan.Init.TimeSeg1 = s_timing[settings->bitrate].bs1;
hcan.Init.TimeSeg1 = s_timing[settings->bitrate].bs2;
Copy link
Member

Choose a reason for hiding this comment

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

Shuold this be timeseg2?


if (!hcanLoc) return BOOTLOADER_ERROR_UNINITIALIZED;

if (msg->id == NULL || msg->extended == NULL || msg->dlc == NULL || msg->data == NULL){
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to check every single variable, maybe just a check for if msg == NULL

@@ -13,6 +13,9 @@
#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>
#include "stm32l433xx.h"
#include "stm32l4xx_hal_can.h"
Copy link
Member

Choose a reason for hiding this comment

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

These should go into Inter-component Headers

(Standard library headers are the ones with <> alligator brakcets around them)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants