Skip to content

Introduce qspi_inst_t type for QSPI instructions #11604

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

Merged
merged 4 commits into from
Oct 18, 2019

Conversation

kyle-cypress
Copy link

Description

Encourage the usage of consistent types (there are currently a mix of int and unsigned int used for qspi instructions)
QSPI commands are limited to 8 bits, to this is a typdef to uint8_t

Pull request type

[ ] Fix
[x] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

@kyle-cypress
Copy link
Author

Split from #11531 , see discussion at #11531 (comment)

@ciarmcom ciarmcom requested review from a team October 1, 2019 01:00
@ciarmcom
Copy link
Member

ciarmcom commented Oct 1, 2019

@kyle-cypress, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

drivers/QSPI.h Outdated
@@ -39,6 +41,10 @@ namespace mbed {
* @{
*/

/** Type representing a QSPI instruction
*/
typedef uint8_t qspi_inst_t;
Copy link
Contributor

@0xc0170 0xc0170 Oct 1, 2019

Choose a reason for hiding this comment

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

qspi_status_t in the HAL defines the instruction as uint8_t. However, the drivers use "fundamental types", see other drivers like SPI. See https://os.mbed.com/handbook/C-Data-Types for details.
Although we use C99 types in HAL.

For consistency, I would leave this as it is or at least go with "unsigned char". We could challenge using these fundamental types in drivers, I recall we were asked previously. Like this one: https://os.mbed.com/questions/1749/What-are-the-preferred-data-types-for-mb/

We should fix the storage if they use unsigned for whatever reason to use the same one.

Copy link
Author

Choose a reason for hiding this comment

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

Updated the typdef to use char instead.

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

Looks good and shouldn't break source backward compatibility.

{
memset(&_qspi_command, 0, sizeof(qspi_command_t));
//Set up instruction phase parameters
_qspi_command.instruction.bus_width = _inst_width;
if (instruction != -1) {
if (instruction != QSPI_NO_INST) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Switch from -1 to 0 was intentional? Is there some other PR which is related to this change?

Copy link
Author

Choose a reason for hiding this comment

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

Since qspi_inst_t is now an unsigned char, -1 is not a valid value. The #define is additionally introduced to improve clarity.
This did however prompt me to check and I noticed that there were two locations where -1 was still being passed to _qspi_build_command; I've pushed up a change updating those to use QSPI_NO_INST.

Copy link
Contributor

@VeijoPesonen VeijoPesonen left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@maclobdell
Copy link
Contributor

@ARMmbed/mbed-os-maintainers - it looks like @0xc0170's requests have been addressed. Can this be merged? @ARMmbed/mbed-os-core - can you review and approve?

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 14, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Oct 14, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 14, 2019

I fixed the build failure directly on your branch, restarting CI

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 14, 2019

Before I start internal tests, I'll wait for Travis, this might need a rebase, will try to do it :)

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 14, 2019

Looks like travis passed (little fs only ongoing), starting CI

@mbed-ci
Copy link

mbed-ci commented Oct 14, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM

@adbridge
Copy link
Contributor

Looks like there are genuine, related CI failures on this PR. @kyle-cypress please investigate

@mbed-ci
Copy link

mbed-ci commented Oct 15, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 4
Build artifacts

drivers/QSPI.h Outdated
@@ -28,6 +28,8 @@

#define ONE_MHZ 1000000

#define QSPI_NO_INST 0x00
Copy link
Contributor

Choose a reason for hiding this comment

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

One additional request, please send new PR: Is this documented anywhere? There are in doxygen notes about: "use this value for this phase to be ignored" . This would be good to have there as well otherwise this define is not known to a user.

Copy link
Author

Choose a reason for hiding this comment

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

Submitted #11691

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 15, 2019

What about the code that used previously -1 for skiping the instruction phase? If they fetch this update, it will be translated to a huge value (won't work), won't it? Shall this be considered breaking change?

Refactor

I would challenge this one to be functionality change rather or even breaking change.

@kyle-cypress
Copy link
Author

What about the code that used previously -1 for skiping the instruction phase? If they fetch this update, it will be translated to a huge value (won't work), won't it? Shall this be considered breaking change?

Refactor

I would challenge this one to be functionality change rather or even breaking change.

Pushed 021ecd0 to change the typedef's underlying type to int instead of char. That way we get the benefits of consistency and clarity from retaining the typedef, but without the backwards compatibility concern.

Matthew Macovsky and others added 4 commits October 16, 2019 15:31
Encourage the usage of consistent types (there are currently
 a mix of `int` and `unsigned int` used for qspi instructions)
QSPI commands are limited to 8 bits, to this is a typdef to char
Existing code may a dependency on the old behavior of "-1" to
mean "no instruction". Therefore, update the typedef, and the value
of QSPI_NO_INST, to avoid breaking those uses.
@kyle-cypress
Copy link
Author

Rebased on latest master to enable rebasing #11531 on this (to test with the changes to qspi_inst_t and QSPI_NO_INST).

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 17, 2019

CI started

So this will need one more rebase ?

@mbed-ci
Copy link

mbed-ci commented Oct 17, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 5
Build artifacts

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 17, 2019

So this will need one more rebase ?

Set as needs: review , otherwise it looks ready

@jeromecoutant
Copy link
Collaborator

Hi
Tested OK with DISCO_L475VG_IOT01A-ARMC6

@kyle-cypress
Copy link
Author

kyle-cypress commented Oct 17, 2019

So this will need one more rebase ?

Set as needs: review , otherwise it looks ready

No, no further rebase is required. The rebase I pushed yesterday is off of master; the comment about #11531 was simply to provide context for the force-push (since there were no merge conflicts that would have prompted a rebase).

@0xc0170 0xc0170 merged commit b6266b5 into ARMmbed:master Oct 18, 2019
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.

10 participants