Skip to content

Add support for target MTB_ADV_WISE_1530. #6423

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 1 commit into from
Closed

Add support for target MTB_ADV_WISE_1530. #6423

wants to merge 1 commit into from

Conversation

KariHaapalehto
Copy link
Contributor

@KariHaapalehto KariHaapalehto commented Mar 22, 2018

Description

Add support for MTB_ADV_WISE_1530 and MTB_USI_WM_BN_BM_22 targets

Pull request type

  • Fix
  • Refactor
  • New target
  • Feature
  • Breaking change

@KariHaapalehto KariHaapalehto deleted the mtb_adv_wise_1530 branch March 22, 2018 10:59
@KariHaapalehto KariHaapalehto restored the mtb_adv_wise_1530 branch March 22, 2018 12:18
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 22, 2018

Can you please share tests (all compilers) ?

0xc0170
0xc0170 previously approved these changes Mar 23, 2018
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Just one nit

GPIO2 = PB_1,
GPIO7 = PB_14,

I2S_CK = PB_12,
Copy link
Contributor

Choose a reason for hiding this comment

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

misligned lines until 190?

Copy link
Contributor

@ashok-rao ashok-rao left a comment

Choose a reason for hiding this comment

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

Suggest to remove USI MTB define from this PR as there are 2 separate targets and the USI needs separate pin defines.
Could you please address other comments. Thanks.

"device_name": "STM32F412RG"
},
"MTB_USI_WM_BN_BM_22": {
"inherits": ["MTB_ADV_WISE_1530"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Derivative target must not inherit from another target. It should inherit from parent MCU..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove usi from here. There will be a lot of duplicate definitions, since the wise-1530 and USI targets are basicly the same targets. According the hw specification the wise-1530 includes usi chip. Only the tx&rx pins are diffrent with these 2 targets

D13 = PA_5,
D14 = PB_9,
D15 = PB_8,

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove all Arduino pin defines. There are no Arduino connectors on the module.

LED_RED = GPIO0,
LED1 = LED_RED,
LED2 = LED_RED,
USER_BUTTON = GPIO2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use standard button names.

LED2 = LED_RED,
USER_BUTTON = GPIO2,
// Not connected
NC = (int)0xFFFFFFFF
Copy link
Contributor

Choose a reason for hiding this comment

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

All pins from the module must be defined. Even the non-connected ones. You may want to move the NC to the top for this.

@@ -100,7 +100,8 @@
defined(TARGET_STM32F439ZI))
#define INITIAL_SP (0x20030000UL)

#elif defined(TARGET_STM32F412ZG)
#elif (defined(TARGET_STM32F412ZG) ||\
defined(TARGET_STM32F412RG))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is required here as F412ZG and RG are differing only in package type..no difference in memory maps between the two variations.
I think the F412RG can be removed from here.. @cmonr @0xc0170 .. is this F412RG required here??

Copy link
Contributor

Choose a reason for hiding this comment

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

@ARMmbed/team-st-mcd Thoughts on the above question?

Copy link
Member

Choose a reason for hiding this comment

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

Hi, The SRAM memory is the same between F412ZG and F412RG. This is the reason why the stack pointer must be initialized at the same address for both Targets.
Please leave this line.

Kind regards

@cmonr
Copy link
Contributor

cmonr commented Mar 26, 2018

Can you please share tests (all compilers) ?

Any update?

@KariHaapalehto
Copy link
Contributor Author

Sw has been build with Jenkins job:
https://jenkins-internal.mbed.com/job/ipcore-cliapp-wiced-test-build/182/

Test's are running at the moment. Will link the results, once the test are done.

@SeppoTakalo
Copy link
Contributor

@0xc0170 What test results are required?
How do we run those?

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 28, 2018

@0xc0170 What test results are required?
How do we run those?

run mbed test for the specific target ( should run all tests for all toolchains) - should be all green. For example, can be seen here #5902 (the first comment, last section contains test results)

@KariHaapalehto
Copy link
Contributor Author

test case results: 4 FAIL / 14 SKIPPED / 461 OK / 2 ERROR

Failing ones:
| MTB_ADV_WISE_1530-GCC_ARM | MTB_ADV_WISE_1530 | mbed-os-tests-mbed_drivers-lp_ticker | FAIL | 30.75 | default |
| MTB_ADV_WISE_1530-GCC_ARM | MTB_ADV_WISE_1530 | mbed-os-tests-mbed_drivers-lp_timeout | TIMEOUT | 267.87 | default |
| MTB_ADV_WISE_1530-GCC_ARM | MTB_ADV_WISE_1530 | mbed-os-tests-mbed_drivers-lp_timer | FAIL | 29.81 | default |
log.txt

| MTB_ADV_WISE_1530-GCC_ARM | MTB_ADV_WISE_1530 | mbed-os-tests-mbed_hal-lp_ticker | TIMEOUT | 47.5 | default |

Test results log-file also attached

@ashok-rao
Copy link
Contributor

@0xc0170 @cmonr .. I'm also seeing these exact failures on another target I'm working on currently. The one I'm working on is also based on the same MCU as this MTB.. might be something related to the MCU timer/ticker implementation?

"inherits": ["FAMILY_STM32"],
"core": "Cortex-M4F",
"extra_labels_add": ["STM32F4", "STM32F412xG", "STM32F412RG", "CORDIO"],
"device_has_add": ["CAN", "LOWPOWERTIMER", "SERIAL_ASYNCH", "SERIAL_FC", "TRNG", "FLASH"],
Copy link
Contributor

Choose a reason for hiding this comment

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

line 1141 and 1153 are overwritten each other both with label device_has_add

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 9, 2018

@0xc0170 @cmonr .. I'm also seeing these exact failures on another target I'm working on currently. The one I'm working on is also based on the same MCU as this MTB.. might be something related to the MCU timer/ticker implementation?

@ashok-rao Has this been reported?

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 9, 2018

Please rebase to resolve the conflict.

If we can report those issues that HAL implementation for a parent has?

@ashok-rao
Copy link
Contributor

@0xc0170 , the failing tests are now reported as issue #6591 ..let's discuss there..thanks!

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.

9 participants