-
Notifications
You must be signed in to change notification settings - Fork 3k
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
PSOC6: initial integration of Cypress HAL #10692
Conversation
@vmedcy, thank you for your changes. |
I just pulled this and tried to build the PSA target version with the following command (using Arm Compiler 6)
I get the following error.
It looks like that file was previously located at C:\Users\maclob01\Documents\mbed-os-5\pe\mbed-os\targets\TARGET_Cypress\TARGET_PSOC6\psoc6_utils.h, but now it is not present anywhere. I'm not sure if it is actually needed or not. |
...es/netsocket/emac-drivers/TARGET_Cypress/TARGET_PSOC6/TARGET_WHD/interface/WhdSTAInterface.h
Outdated
Show resolved
Hide resolved
...es/netsocket/emac-drivers/TARGET_Cypress/TARGET_PSOC6/TARGET_WHD/interface/WhdSTAInterface.h
Outdated
Show resolved
Hide resolved
features/netsocket/emac-drivers/TARGET_Cypress/TARGET_PSOC6/TARGET_WHD/interface/whd_emac.cpp
Outdated
Show resolved
Hide resolved
features/netsocket/emac-drivers/TARGET_Cypress/TARGET_PSOC6/TARGET_WHD/interface/whd_emac.cpp
Outdated
Show resolved
Hide resolved
features/netsocket/emac-drivers/TARGET_Cypress/TARGET_PSOC6/TARGET_WHD/network/whd_network.h
Outdated
Show resolved
Hide resolved
features/lwipstack/lwipopts.h
Outdated
#ifndef LWIP_RAW | ||
#define LWIP_RAW 0 | ||
#endif | ||
#define LWIP_RAW 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes were added for PSOC6 testing but it is not clear if they should remain here as they affect all targets.
@vmedcy @morser499
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not, this PR can be dependent on it (will be treated like that if required). I already mentioned few other issues like this, please create separate PR. They will get much faster in and easier to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LWIP_RAW change moved to #10978
@hennadiykytsun - perhaps this PR needs to be rebased to the latest master then, to avoid possibility that PR 10747 gets reverted? I will let the maintainers determine if that is necessary. |
@maclobdell - I have verified each Cypress target in this pull request - each of them has fix from the latest master. All asm files are correct. |
This should be the complete set of changes for the release. |
Are all commits ready for release (no need to clean - squash/rebase) ? I can see some merge commits in the history, they cause issues in patch releases. Is this targeting the next minor release? |
@0xc0170: yes, this PR is ready for release, targeting 5.13.1. Can you clarify your request to remove merge commits: is rebase required? |
If this is targeting the patch release (the next one will be 5.13.2), then yes. Cherry-pick will be difficult with merge commits |
Rebased against latest ARMmbed/master (this removed merge commits) |
I triggered CI run, will review tomorrow |
Test run: FAILEDSummary: 1 of 4 test jobs failed Failed test jobs:
|
I reviewed the fail log: mbed-os-greentea_FUTURE_SEQUANA_ARM.log. The error is related to FUTURE_SEQUANA targets inheriting TARGET_PSOC6 but not implementing DEVICE_I2CSLAVE and DEVICE_SPISLAVE. Fixing targets.json to add device_has_remove appropriately. |
Removed DEVICE_I2CSLAVE for FUTURE_SEQUANA targets in b652407. |
ci restarted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through the code and left couple of questions. Those won't be blocking this PR.
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
@vmedcy We are having problems patching this across to the 5.13 release branch, getting these errors: Are we sure there are no dependencies on any other PR on master that this is sitting on top of ? |
There now |
I've tried applying as a full patch and by cherry picking individual commits , both ways get stuck at that same commit |
@adbridge How should @cypress-neil show his work on the cherry-picking and manual resolution? A new PR? |
I suggest making an equivalent PR to the mbed-os-5.13 branch for these changes. That may well then need rebasing once we drop the rest of the patch release there |
To address the conflict with 'remove PSA targets' the PSA targets should be removed from the targets.json file and the 5 .hex files should be deleted. |
@morser499 can you guys please create a new PR directly against the mbed-os-5.13 branch, perhaps combining this PR with 11027 so we can bring both across easily ? NOTE we wouldn't normally do it this way but I think it will be the quickest way to try and ensure both get into the patch release. |
@adbridge Ryan and his team will be working on the new PR starting in about 30 minutes or so. |
Created #11030 |
@adbridge Neil has created the requested PR and linked it above. Thanks. |
I came across this PR today while searching for answer why some PSA targets were removed - in this PR. There is a commit "PSOC6: remove PSA targets" - does not explain why it is being removed. Please in the future, add more content to the commit message. I don't understand how "initial integration of Cypress HAL" is related to removing targets. Removing targets is not considered only "a target update" but rather functionality change - whoever used this target won't be able to use it after the release 5.13.1. We need to be more careful with changes like this and do not mix multiple PR types to avoid the surprises like this one. |
This breaks our CI now, as we are using CY8CKIT_062_WIFI_BT_PSA as one of our targets. I am not really sure was this intended? |
Description
This PR brings the following major updates to the Cypress PSoC 6 targets for Mbed OS 5.13:
Pull request type
Reviewers
@ARMmbed/team-cypress
Release Notes