Skip to content

Fix SmartCard HAL on STM32 BluePill board. #5303

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

Closed
wants to merge 2 commits into from

Conversation

balika011
Copy link

Description

The current HAL_SMARTCARD_Receive function corrupts the memory. Changes pData[Size] to 0. That's outside of the pData. Also the MSP is missing and that's needed for smartcard hal to work.

Status

READY

@@ -530,8 +529,7 @@ HAL_StatusTypeDef HAL_SMARTCARD_Receive(SMARTCARD_HandleTypeDef *hsc, uint8_t *p
{
return HAL_TIMEOUT;
}
tmp = (uint16_t*) pData;
*tmp = (uint8_t)(hsc->Instance->DR & (uint8_t)0xFF);
*pData = (uint8_t)(hsc->Instance->DR & (uint8_t)0xFF);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is changing hal directly, will this be overwritten by next hal update?

cc @LMESTM @jeromecoutant

Copy link
Collaborator

Choose a reason for hiding this comment

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

Internal ticket reference : 39116

Copy link
Contributor

Choose a reason for hiding this comment

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

Internal ticket reference : 39116

What does this mean, that will be fixed in the next update. What about this change, +1 from your side?

Copy link
Contributor

Choose a reason for hiding this comment

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

This means that this part of change is being reviewed internally. But from review the change makes sense so I think we can accept this change in a first step and we will update it later if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi
Just received confirmation from driver team that patch in stm32f1xx_hal_smartcard.c is needed.
Thx for issue reporting

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 13, 2017

@balika011 Can you please sign https://os.mbed.com/contributor_agreement/ ? I could not locate your nick on https://os.mbed.com page

@balika011
Copy link
Author

Done.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 19, 2017

@jeromecoutant Happy with this patch to go in?

Copy link
Contributor

@LMESTM LMESTM left a comment

Choose a reason for hiding this comment

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

The MSP changes should be kept in the user application side. For the fix in HAL, +1 from me !

@@ -10630,6 +10630,24 @@ typedef struct
* @}
*/

#define SC_USART USART3
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer that this change coming from MSP layer do not end in device/ directory.
Those files are application layer dependent as other application may want other pins than PB_10 / PB_11. So I think that you should rather add them to your project only

@@ -0,0 +1,127 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - if really needed this file stm32f1xx_hal_msp.c belongs to application layer.

@@ -530,8 +529,7 @@ HAL_StatusTypeDef HAL_SMARTCARD_Receive(SMARTCARD_HandleTypeDef *hsc, uint8_t *p
{
return HAL_TIMEOUT;
}
tmp = (uint16_t*) pData;
*tmp = (uint8_t)(hsc->Instance->DR & (uint8_t)0xFF);
*pData = (uint8_t)(hsc->Instance->DR & (uint8_t)0xFF);
Copy link
Contributor

Choose a reason for hiding this comment

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

This means that this part of change is being reviewed internally. But from review the change makes sense so I think we can accept this change in a first step and we will update it later if needed.

@theotherjimmy
Copy link
Contributor

@balika011 Could you address @LMESTM's comments?

@LMESTM
Copy link
Contributor

LMESTM commented Oct 23, 2017

@balika011 BTW, sorry for sending my feedback so late ... I hope you're ok to do the proposed changes. And thanks a lot for reporting and fixing the HAL issue !

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 13, 2017

@balika011 Do you need any assistance for the proposed changes?

@LMESTM When the HAL receives these fixes? Shall we fix this separately to move forward this proposal?
I would propose to move things forward - to cherry-pick HAL changes from here, mainline them to get this into the codebase.

@LMESTM
Copy link
Contributor

LMESTM commented Nov 13, 2017

@0xc0170 Sure - will do so in coming days ...

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 15, 2017

@balika011 Please review #5505 , will close this one

@0xc0170 0xc0170 closed this Nov 15, 2017
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.

5 participants