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

Nuvoton: Fix FPGA CI test failing #11152

Merged
merged 15 commits into from Aug 23, 2019

Conversation

@ccli8
Copy link
Contributor

commented Aug 2, 2019

Description

This PR is to pass FPGA CI test shield tests on Nuvoton targets. It has the following major bugfix:

  • Skip some Nuvoton targets not supporting pull-up/pull-down input mode
  • Force enum PinName to 32-bit required to encode module binding information in it
  • Exclude USB UART from testing which is dedicated to USB VCOM
  • Fix IP initialization sequence
  • Fix I2C NACK error
  • Fix redundant SPI clock generation
  • Fix redundant call to UART IRQ handler
  • Support GPIO input pull-high/pull-low (not all Nuvoton targets support it)
  • Other target-specific bugfix

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change
@ciarmcom ciarmcom requested review from Ronny-Liu and ARMmbed/mbed-os-maintainers Aug 2, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

@ccli8 Please fix that Astyle error.
Seems to be just one whitespace problem.

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_fpga-ci branch from 475a64f to e8c2210 Aug 6, 2019
@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Make the following modifications:

  1. Do rebase
  2. Fix astyle
  3. Remove FPGA: Skip Nuvoton targets for SPI 4-bit symbol size test because master has removed 4/12-bit symbol size test
  4. Exclude A2/A3 from testing for NuMaker-IoT-M487 V1.3 board
@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_fpga-ci branch from 8f4a551 to 5539954 Aug 6, 2019
// Some targets don't support pull-up/pull-down mode.
#if !defined(TARGET_NANO100) && \
!defined(TARGET_NUC472) && \
!defined(TARGET_M451)

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Aug 8, 2019

Member

How can we eliminate these if target. Is this functionality a must ? Can we check differently if this mode is supported?

Having this ifdef is not ideal, means we are testing something is not either supported by all targets or we are missing a check to ask for "is this supported, test it".

This comment has been minimized.

Copy link
@ccli8

ccli8 Aug 13, 2019

Author Contributor

Check gpio_mode(...) and pin_mode(...) but find no spec on the mode parameter. It seems most targets support PullDown/PullUp input pull mode. However, old-chip based Nuvoton targets like above don't support it. And there's no configuration option to disable it.

void gpio_mode(gpio_t *obj, PinMode mode);
void pin_mode(PinName pin, PinMode mode);

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Aug 14, 2019

Member

@ARMmbed/mbed-os-hal Does this look correct as it is?

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Aug 19, 2019

Member

@ccli8 The whole // Test input, pull-up mode. should be excluded if these targets do not support pull-up mode.

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Aug 19, 2019

Member

How can we eliminate these if target. Is this functionality a must ? Can we check differently if this mode is supported?

Having this ifdef is not ideal, means we are testing something is not either supported by all targets or we are missing a check to ask for "is this supported, test it".

Frankly, I didn't expect any target that does not support pull-up/pull-down modes. The only thing that comes to my mind, to make this a bit more readable, is to define a new macro in targets.json file. That could be either "GPIO_PINMODE" in "device_has" section or "GPIO_PINMODE_NOT_SUPPORTED" in "macros". @ccli8, @0xc0170, @OPpuolitaival, what do you think?

This comment has been minimized.

Copy link
@ccli8

ccli8 Aug 20, 2019

Author Contributor

Before that, I think the mode parameter should be well defined. It just allows PullNone/PullUp/PullDown, or it can also allow other modes like target-specific? Only meaningful for input mode?

void gpio_mode(gpio_t *obj, PinMode mode);
void pin_mode(PinName pin, PinMode mode);
// Some targets don't support pull-up/pull-down mode.
#if !defined(TARGET_NANO100) && \
!defined(TARGET_NUC472) && \
!defined(TARGET_M451)

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Aug 19, 2019

Member

@ccli8 The whole // Test input, pull-up mode. should be excluded if these targets do not support pull-up mode.


// Test input, pull-down mode.
gpio_mode(&gpio, PullDown);
tester.gpio_write(MbedTester::LogicalPinGPIO0, 0, false);
wait_us(HI_Z_READ_DELAY_US);
// Some targets don't support pull-up/pull-down mode.

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Aug 19, 2019

Member

Same as above -- the whole // Test input, pull-down mode. should be excluded if pull-down is not supported.

This comment has been minimized.

Copy link
@ccli8

ccli8 Aug 20, 2019

Author Contributor

@fkjagodzinski Including above, fixed as commented

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Aug 20, 2019

Member

Thanks for a quick update. Don't you think the #if statement is a bit too wide now? I thought that only the pull-up and pull-down modes are unsupported; pull-none was OK, wasn't it? I'd rather see these lines enclosed in #if !defined(TARGET_NANO100) .... I do not have these targets available for local tests, so will let you decide. 😉

This comment has been minimized.

Copy link
@ccli8

ccli8 Aug 20, 2019

Author Contributor

@fkjagodzinski Chip may have fixed (non-configurable) input pull mode on these targets. To be not confusing, pull modes (PullNone/PullUp/PullDown) are not supported on these targets.

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Aug 22, 2019

Member

In that case, we can leave the #if statements as they are.

// Some targets don't support pull-up/pull-down mode.
#if !defined(TARGET_NANO100) && \
!defined(TARGET_NUC472) && \
!defined(TARGET_M451)

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Aug 19, 2019

Member

Since every step of this test (test_explicit_input()) sets one of the PinModes, the whole test should be skipped if not supported. I think the #if statement should be moved to the declaration of cases below.

This comment has been minimized.

Copy link
@ccli8

ccli8 Aug 20, 2019

Author Contributor

@fkjagodzinski Fixed as commented

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_fpga-ci branch from 5539954 to 2d93f50 Aug 20, 2019
ccli8 added 15 commits Aug 20, 2019
…n mode

The Nuvoton targets listed below don't support input pull-up/pull-down mode
and so are skipped for test:
- NUMAKER_PFM_NANO130
- NUMAKER_PFM_NUC472
- NUMAKER_PFM_M453
The most suitable place to free up I2C pins is in i2c_free(). Due to
i2c_free() not available in I2C HAL, we free up I2C pins manually by
configuring them back to GPIO.

Without free-up of I2C pins, SDA/SCL pins of the same I2C peripheral may
share by multiple ports due to 'all ports' tests here, and the following I2C
tests would be subject to interference by shared ports.
USB UART is dedicated to USB COM and so must exclude from FPGA CI testing.
NU_PINNAME_BIND(...) requires enum PinName to be 32-bit to encode module
binding information in it.
Better IP initialization sequence:
1. Configure IP pins
2. Select IP clock source and then enable it
3. Reset the IP (SYS_ResetModule)

NOTE1: IP reset takes effect regardless of IP clock. So it doesn't matter if
       IP clock enable is before IP reset.
NOTE2: Non-configured pins may disturb IP's state, so IP pinout first and then
       IP reset.
NOTE3: IP reset at the end of IP initialization sequence can cover unexpected
       situation.
Fix logic error on replying NACK at the end of transfer.

This is also to fix FPGA CI test mbed_hal_fpga_ci_test_shield-i2c/
i2c - test single byte read i2c API.
Fix SPI clocks are generated redundantly at the end of transfer.

This is also to fix FPGA CI test mbed_hal_fpga_ci_test_shield-spi/
SPI - async mode.
Honor RxIrq/TxIrq to avoid redundant call to UART IRQ handler.

This is also to fix FPGA CI test mbed_hal_fpga_ci_test_shield-uart.
In Nuvoton, only new-design chips support GPIO input pull-high/pull-low modes.
Targets not supporting this feature are listed below:

- NUMAKER_PFM_NANO130
- NUMAKER_PFM_NUC472
- NUMAKER_PFM_M453
MOSI1/MISO1 are used in second bit of 2-bit transfer mode and cannot be used
for normal MOSI/MISO. Remove them from pinmap.

This is also to fix FPGA CI test mbed_hal_fpga_ci_test_shield-spi/
SPI - basic test.
M451 series can classify by M45xD/M45xC and M45xG/M45xE. To support this
classification:
1.  Create TARGET_M45xD_M45xC and TARGET_M45xG_M45xE targets.
2.  Mark NUMAKER_PFM_M453 belongs to TARGET_M45xG_M45xE by 'extra_labels_add'
    in targets.json.
3.  Fix pin name table according to the classification.
4.  Fix pinmap table according to the classification.
Without free-up of peripheral pins, peripheral pins of the same peripheral may
share by multiple ports after port iteration, and this peripheral may fail with
pin interference.
Fix SPI module index error in modidx_ns_tab table in CLK_SetModuleClock_S().
Need to update secure image for this bugfix.

This is also to fix FPGA CI test mbed_hal_fpga_ci_test_shield-spi/
SPI - init/free test all pins.
Since NuMaker-IoT-M487 V1.3, A2/A3 are dedicated to on-board ESP8266 WiFi
module RTS/CTS pins and so must exclude from FPGA CI testing.
@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_fpga-ci branch from 2d93f50 to 96dac4f Aug 20, 2019
@0xc0170 0xc0170 self-requested a review Aug 20, 2019
Copy link
Member

left a comment

TESTS/mbed_hal_fpga_ci_test_shield/gpio/main.cpp change still needs to be approved by @fkjagodzinski

@0xc0170 0xc0170 added needs: work and removed needs: review labels Aug 20, 2019
@fkjagodzinski

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

TESTS/mbed_hal_fpga_ci_test_shield/gpio/main.cpp change still needs to be approved by @fkjagodzinski

I've just left one last comment to consider.

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

@fkjagodzinski @0xc0170 Any update?

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Aug 22, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 22, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit 48b5939 into ARMmbed:master Aug 23, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(-24 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8524 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170 0xc0170 removed the needs: CI label Aug 23, 2019
@adbridge

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

This is sitting on top of 5.14 code so now targeting 5.14 instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.