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

SDT64, 8195, 32620, 32625, 51822, 52832B added to targets #7669

Merged
merged 10 commits into from
Aug 17, 2018

Conversation

jwyune
Copy link

@jwyune jwyune commented Aug 1, 2018

Description

We are adding six new boards to mbed-os. The list is as follows:

  • SDT64B
  • SDT8195B
  • SDT32620B
  • SDT32625B
  • SDT51822B
  • SDT52832B

Pull request type

[ ] Fix
[ ] Refactor
[x] New target
[ ] Feature
[ ] Breaking change

@jwyune
Copy link
Author

jwyune commented Aug 1, 2018

We tested the boards with GCC_ARM only.

Test results

  1. Blinky (https://github.com/ARMmbed/mbed-os-example-blinky): all six boards successful
  2. iBeacon (https://os.mbed.com/teams/mbed-os-examples/code/mbed-os-example-ble-Beacon/): SDT51822B and SDT52832B successful
  3. Ethernet (https://os.mbed.com/docs/v5.9/reference/ethernet.html): SDT64B successful
  4. Wi-Fi (https://github.com/ARMmbed/mbed-os-example-wifi): SDT8195B successful

@jwyune
Copy link
Author

jwyune commented Aug 2, 2018

I attached Greentea results for all the six boards.

SDT64B.txt
SDT8195B.txt
SDT32620B.txt
SDT32625B.txt
SDT51822B.txt
SDT52832B.txt

0xc0170
0xc0170 previously requested changes Aug 2, 2018
@@ -635,6 +635,23 @@
"network-default-interface-type": "ETHERNET"
}
},
"SDT64B": {
"core": "Cortex-M4F",
Copy link
Contributor

Choose a reason for hiding this comment

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

why this target do not inherit from MCU K64F ? IT looks like it would benefit and override few details .

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to other targets below.

Copy link
Author

Choose a reason for hiding this comment

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

I will simplify "SDT64B", but we cannot simplify the codes for the other boards. I will explain the reason in the next comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, please do !

Ideally, K64F and some other targets contain base target (MCU_xxx) that we can inherit from. So if there are targets that do not have this hierarchy, they do not allow us to inherit from and set-up own target

@@ -4061,6 +4105,25 @@
"network-default-interface-type": "WIFI"
}
},
"SDT8195B": {
"core": "Cortex-M3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this targetr does not reuse realtek - I can see new files here (HAL implemention) - arent these the same as for realtek?

Copy link
Author

Choose a reason for hiding this comment

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

If I just do "inherits": ["REALTEK_RTL8195AM"], I get pin name declaration errors. We see the same error for all the boards, except for SDT64B.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the problem here

@ARMmbed/team-realtek This target is a platform and can't be used as base (MCU_RTL..... would be needed here?). Please review

Copy link
Author

Choose a reason for hiding this comment

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

@0xc0170 There is another issue with K64F. The folder that contains EMAC for K64F is also "TARGET_K64F" (https://github.com/ARMmbed/mbed-os/tree/master/features/netsocket/emac-drivers/TARGET_Freescale_EMAC/TARGET_K64F). This can be pretty confusing to users, because if you want to use the ethernet driver, you need to add "extra_labels_add": ["K64F"].

Target "K64F" should have added "extra_labels_add": ["K64F"], but they didn't have to, because their target name also "happens" to be the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that EMAC implementation generic for any K64F or targetting just frdm-k64f board? If generic, folder should be named TARGET_MCU_K64F (no addition should be needed there, but MCU_K64F is not yet a target but at least there are folders that contain MCU files). Can you create an issue for this? This should be reviewed

@jwyune
Copy link
Author

jwyune commented Aug 2, 2018

@0xc0170 We couldn't simplify the targets.json codes for the other boards. If we inherit REALTEK_RTL8195AM, MAX32620FTHR, NRF51_DK, and NRF52_DK, pin declaration error would occur, because mbed-os would look for pin names in those folders, not in the SDT*B folders.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 3, 2018

I'll create an issue for REALTEK_RTL8195AM if t here can be MCU_ target created that one of these new targets could inherit from to get boilerplate.

Regarding NRF51_DK - that one is OK for now, would need refactor.

NRF52_DK - inherits from MCU_NRF52832 - MCU_NRF52832 should be your parent then, to inherit from.

@jwyune
Copy link
Author

jwyune commented Aug 3, 2018

We are using neither of NRF51_DK nor NRF52_DK. SDT51822B inherits from MCU_NRF51_32K_UNIFIED, and SDT52832B from MCU_NRF52832. Those two boards meet the requirements.

GPIO5 = PTC6,
GPIO6 = NOT_CONNECTED,

//Purse Width Modulation (PWM)
Copy link
Contributor

Choose a reason for hiding this comment

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

Purse => Pulse (typo)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing this out.

UART2_TX = PTC17,
UART2_CTS = PTC19,
UART2_RTS = PTC18,

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please define default peripherals for each type? Ex: UART_TX = UART0_TX , I2C_SDA = I2C1_SDA and so on..

Copy link
Author

Choose a reason for hiding this comment

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

Where can I find the names for default peripherals? I see LED1, 2, 3, and USBTX, RX, but others don't seem to have that. For example, https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_Maxim/TARGET_MAX32620C/TARGET_MAX32620FTHR/PinNames.h

Copy link
Author

Choose a reason for hiding this comment

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

@ashok-rao I'll define the following list:
USBTX, USBTX, LED1, LED2, LED3, LED_RED, LED_GREEN, LED_BLUE, SERIAL_TX, SERIAL_RX, I2C_SCL, I2C_SDA, SPI_MOSI, SPI_MISO, SPI_SCK, SPI_CS, PWM_OUT

Anything else?

Copy link
Contributor

@ashok-rao ashok-rao Aug 6, 2018

Choose a reason for hiding this comment

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

That should do it! Thanks! (Same for all the below).

SPI3_SS0 = P5_3,
SPI3_SS1 = P5_4,
SPI3_SS2 = P5_5,

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments as above regarding default peripherals. Thanks.

SPI3_SS0 = NOT_CONNECTED,
SPI3_SS1 = NOT_CONNECTED,
SPI3_SS2 = NOT_CONNECTED,

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments as above regarding default peripherals.

SPI3_SS0 = NOT_CONNECTED,
SPI3_SS1 = NOT_CONNECTED,
SPI3_SS2 = NOT_CONNECTED,

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above for default peripherals.

UART2_TX = NOT_CONNECTED,
UART2_CTS = NOT_CONNECTED,
UART2_RTS = NOT_CONNECTED,

Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above regarding default peripherals.

I2C1_SDA = PB_3,

I2C2_SCL = NOT_CONNECTED,
I2C2_SDA = NOT_CONNECTED,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment for default peripheral, but looks like only I2C needs to have one in this case.

@jwyune
Copy link
Author

jwyune commented Aug 6, 2018

@ashok-rao I added generic pin names for all the boards. Thanks.

@cmonr
Copy link
Contributor

cmonr commented Aug 14, 2018

Just making a note here that before this can come in, the config will need to derive off of a base target, which is what #7704 will provide.

@jwyune
Copy link
Author

jwyune commented Aug 15, 2018

@cmonr @ashok-rao We are launching our boards very soon, and delay in "mbed--os" would be a big problem for us, because it also delays "Mbed Enablement" certification process. Delay here is actually hurting our business at the moment.

How about this? I can easily remove everything related to RTL8195, and you can get everything else merged first. Note that we don't have more work to do with other boards. Please let me know what you think. I made this PR two weeks ago, and we really need this to be approved as soon as possible.

@ashok-rao
Copy link
Contributor

Sounds good to me .. @cmonr , @0xc0170 : What do you guys say? I don't see any blockers with the other targets in this PR except the RTL8195 having dependency on 7704..

@cmonr
Copy link
Contributor

cmonr commented Aug 15, 2018

@jwyune Thank you for the time pressure info. We can go ahead with your plan to get this PR in sooner rather than later. Once #7704 comes in though, we would appreciate refactoring so that they derive from the new target.

@andrewc-arm
Copy link
Contributor

Hi, I agree to @jwyune suggestion. Let's submit all the board code except Realtek. After this, we can proceed to Mbed Enabled process. Thanks.

@jwyune
Copy link
Author

jwyune commented Aug 16, 2018

Hello, @andrewc-arm @ashok-rao @cmonr

I removed everything related to RTL8195, but I see a few test fails. How can I reproduce the failed results locally to fix those?

@cmonr
Copy link
Contributor

cmonr commented Aug 16, 2018

@jwyune Take a look at the .travis.yml file at the base of the Mbed OS directory. Inside of it, you'll find all of the commands that are run in Travis CI.

The cloud_client_smoke_test looks like it's internal only, but it looks like it's failing for similar reasons that the other tests are, being that there's a Json key error with 'MAX32625_NO_BOOT'

@jwyune
Copy link
Author

jwyune commented Aug 16, 2018

@cmonr That's bizarre, because I changed nothing, but removed everything related to RTL8195.

@ashok-rao
Copy link
Contributor

ashok-rao commented Aug 16, 2018

Looks like travis is passing now.. let's wait for CI to finish.. @cmonr ..can you trigger it please?

@jwyune
Copy link
Author

jwyune commented Aug 16, 2018

@ashok-rao
crossed-fingers

@cmonr
Copy link
Contributor

cmonr commented Aug 16, 2018

Will start in a bit. The Test CI queue was reeealy long in the last 48 hours.

@cmonr cmonr dismissed 0xc0170’s stale review August 16, 2018 13:02

Path forward for now. Comments will be addressed once dependent PR is mergedm, but this PR will not wait in the short term.

#ifndef MBED_DEVICE_H
#define MBED_DEVICE_H


Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for all of this white space?

Copy link
Author

Choose a reason for hiding this comment

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

Not really... This file was directly copied from TARGET_K64F.

@cmonr
Copy link
Contributor

cmonr commented Aug 16, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 16, 2018

Build : SUCCESS

Build number : 2819
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7669/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Aug 16, 2018

Exporter Build : SUCCESS

Build number : 2450
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7669/

@mbed-ci
Copy link

mbed-ci commented Aug 17, 2018

Test : SUCCESS

Build number : 2567
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7669/2567

@cmonr cmonr merged commit 8e25d2d into ARMmbed:master Aug 17, 2018
pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018
SDT64, 8195, 32620, 32625, 51822, 52832B added to targets
@0xc0170
Copy link
Contributor

0xc0170 commented Aug 23, 2018

Just a note, this caused issue referenced above #7829 - ipv6 builds fail as there are dhcp files added in this PR. What was the intention ? We are reverting them to fix the regression.

@cmonr
Copy link
Contributor

cmonr commented Aug 24, 2018

@0xc0170 I added a release version label for the last patch, since 5.9.6 is already made and undergoing testing.

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