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: read can do only up to 8 bytes, error if more is reported #15374

Merged

Conversation

0xc0170
Copy link
Contributor

@0xc0170 0xc0170 commented Jan 16, 2023

Summary of changes

If HAL implementation writes more than 8 bytes of data, error immediately. CANMessage defines only 8 bytes of data, lenght cannot be > 8.

This fixes #15361

@Martyx00 Please review

Impact of changes

Migration actions required

Documentation


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[x] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


If HAL implementation writes more than 8 bytes of data, error immediately.
CANMessage defines only 8 bytes of data, lenght cannot be > 8.

This fixes ARMmbed#15361

Signed-off-by: Martin Kojtal <[email protected]>
@0xc0170 0xc0170 changed the title CAN: read only up to 8 bytes CAN: read can do only up to 8 bytes, error if more is reported Jan 16, 2023
@0xc0170
Copy link
Contributor Author

0xc0170 commented Jan 16, 2023

I'll check if we need the same fix for 5.15 branch, if yes, I'll create new PR as well.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Jan 17, 2023

CI startd

@Martyx00
Copy link
Contributor

The only issue that I can see is that if the FD CAN is used:

than the data could be up to 64 bytes (https://www.can-cia.org/can-knowledge/can/can-fd/). If I am not mistaken, currenly MbedOS does not support FD CAN. However, when it will, it will be necessary to not only extend the size of the data field in the CAN_Message structure
unsigned char data[8]; // Data field
, but also adjust this condition.

@mbed-ci
Copy link

mbed-ci commented Jan 17, 2023

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170
Copy link
Contributor Author

0xc0170 commented Jan 17, 2023

The only issue that I can see is that if the FD CAN is used:

@Martyx00 correct, it will require rework in the driver itself (contstructor limites also lenght to maximum 8) and assumping the message is predefined to be just 8 bytes in the driver, all HAL implementations use 8 bytes mode only (if they do not, we will catch it via this error).

I wonder if this FDCAN1 in HAL makes sense as if a user would something define this, it could lead to similar issues that this PR is fixing - however we will catch it with this fix so overflow should not happen
@jeromecoutant is this correct or I misunderstood FDCAN1 in CAN HAL?

@jeromecoutant
Copy link
Collaborator

If I am not mistaken, currenly MbedOS does not support FD CAN.

I don't know CAN very much... but I thought that ST FD CAN was OK in mbed-os.

#ifdef FDCAN1

In all STM32 newest families, FDCAN IP has replaced CAN IP.

@Martyx00
Copy link
Contributor

Not sure if MbedOS officially supports FD CAN. But none of the boards mentioned here: https://os.mbed.com/platforms/?q=&Communication=CAN have FD CAN. I have the Nucleo with the L5 chip somewhere around so I can test if it is possible to use FD CAN with MbedOS in coming days. If it is possible, than there is likely to be some trouble due to the size restriction of the CAN_Message (it may take some time as I have to also find a second board that can send the FD CAN frames - I only have one L5 Nucleo).

@Martyx00
Copy link
Contributor

Did a quick check in the source code, FD CAN does not seem to be supported. Any limitations of this PR should be taken into account when implementing it in the future.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Jan 18, 2023

@Martyx00 happy with this pull request?

@Martyx00
Copy link
Contributor

@Martyx00 happy with this pull request?

Yes

@0xc0170 0xc0170 requested a review from saheerb January 18, 2023 10:02
@0xc0170 0xc0170 added release-type: patch Indentifies a PR as containing just a patch needs: review labels Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CAN DLC field allows values higher than 8 in standard mode
6 participants