Skip to content

Add CC3220SF LAUNCHXL to Mbed OS #11041

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

linlingao
Copy link
Contributor

Description

Add CC3220SF LAUNCHXL to the Mbed OS family.

Pull request type

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

Reviewers

Release Notes

@ciarmcom ciarmcom requested review from bentcooke, maclobdell and a team July 12, 2019 19:01
@ciarmcom
Copy link
Member

@linlingao, thank you for your changes.
@bentcooke @maclobdell @ARMmbed/mbed-os-core @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-pan @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-test @ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-connectivity @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-tls please review.

@@ -107,7 +107,7 @@ void lp_ticker_info_test()

TEST_ASSERT(p_ticker_info->frequency >= 4000);
TEST_ASSERT(p_ticker_info->frequency <= 64000);
TEST_ASSERT(p_ticker_info->bits >= 12);
TEST_ASSERT(p_ticker_info->bits >= 12 && p_ticker_info->bits <= 32);
Copy link
Member

Choose a reason for hiding this comment

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

Why are we changing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The TI HW timer is 48bit. I initially set it to 48bit and ran into an issue. I reverse-engineered it and found out Mbed OS really just supports less than or equal to 32bit.

@@ -501,7 +501,7 @@ utest::v1::status_t greentea_failure_handler(const Case *const source, const fai
// Test setup
utest::v1::status_t test_setup(const size_t number_of_cases)
{
GREENTEA_SETUP(60, "default_auto");
GREENTEA_SETUP(240, "default_auto");
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 like that, it's 4x the current value. Not knowing much about the the reasons I would say something else is going wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test was timing out with the old value. I'll retested this with the latest code.

@@ -0,0 +1,14 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this XML file? How is it used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used by Code Composer Studio debugger. It tells the debugger what address ranges are valid.

"CC3220SF_LAUNCHXL": {
"inherits": ["CC3220SF"],
"components_add": ["SD", "FLASHIAP"],
"device_has": ["USTICKER", "SERIAL", "SERIAL_FC", "PORTIN", "PORTINOUT", "PORTOUT", "PWMOUT", "INTERRUPTIN", "SPI", "ANALOGIN", "FLASH", "TRNG", "RTC"],
Copy link
Member

Choose a reason for hiding this comment

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

We don't implement sleep or lpticker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sleep has not been implemented. I don't think it's a required feature in Mbed OS, but if there's a demand down the road, we can add it.
I implemented LPTICKER code but the long latency in their HW design makes it useless in Mbed OS.

@artokin
Copy link
Contributor

artokin commented Jul 15, 2019

@linlingao , would you please rebase your work on top of the master branch? Then it will be much easier to review.

@linlingao
Copy link
Contributor Author

I've rebased 137 commits on top of the master. What a nightmare! I tried to squash the remaining 5 commits into one but ran into some issues. I think I've stretched github too thin. It shouldn't affect review/merge.

@40Grit
Copy link

40Grit commented Jul 16, 2019

Between this, psoc, the c++ and rtos api improvements; There seem to be a lot of big ones lately. Wondering what the trend is.

@SeppoTakalo
Copy link
Contributor

History has a merge commits, cannot be accepted.

You need to rebase your work on top of master, not merge from master.
https://git-scm.com/book/en/v2/Git-Branching-Rebasing

If you already rebased, and you had a merge commits, it becomes nightmare. Do a clean branch with only your changes.

@linlingao
Copy link
Contributor Author

@SeppoTakalo The rebase of 137 commits appears to be too complex for Git (and me) to handle cleanly. I kept running into weird conflicts. I have submitted another one in #11063.
Closing this one. My apology to Bartek for having reviewed this PR. Thankfully the remaining 12 reviewers haven't started it.

@linlingao linlingao closed this Jul 17, 2019
@linlingao
Copy link
Contributor Author

@40Grit I think it's just coincidence. Hopefully the trend is, Mbed OS supports more boards with improved quality :).

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.

6 participants