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

[WIZnet][W7500*] Support for mbed OS 5 #4226

Merged
merged 6 commits into from May 15, 2017

Conversation

Projects
None yet
6 participants
@khj098765
Contributor

khj098765 commented Apr 26, 2017

Description

For support mbed OS 5 PR
Modifying the code for mbed ci shield test -> analogin_api.c, W7500x_adc.c, gpio_irq_api.c, W7500x_gpio.c
Modifying the code for fix bug -> W7500x_uart.c(register control driver problem), W7500x_uart.h, pinmap.c(pullup, pulldown problem)

Status

READY

Migrations

NO

Modifying the code to support mbed OS 5 -> add mbed_rtx.h
Modifying the code for mbed ci shield test -> analogin_api.c, W7500x_adc.c, gpio_irq_api.c, W7500x_gpio.c
Modifying the code for fix bug -> W7500x_uart.c(register control driver problem), W7500x_uart.h, pinmap.c(pullup, pulldown problem)
@andrewc-arm

This comment has been minimized.

andrewc-arm commented Apr 26, 2017

Hi,

I am the Partner Enablement Korea lead for WIZnet.
This PR is very important due to WIZnet/ARM co-marketing event which is due on May 1st.
WIZnet has done all the pretesting (Greentea + CI-test) and analysis already.
PE team and WIZnet have agreed on waiver of RTC and PWM test.
Except those, everything else works well.

Please let me know how we can expedite this process.
And due to the urgency, I will be co-leading this issue actively.

Thanks,

@@ -42,6 +42,7 @@ void ADC_InterruptClear (void)
void ADC_Init (void)
{
// ADC_CLK on
ADC_PowerDownEnable(ENABLE);

This comment has been minimized.

@0xc0170

0xc0170 Apr 26, 2017

Member

please keep the indentation 4 spaces as it was (or this might be a tab ?)

This comment has been minimized.

@khj098765

khj098765 Apr 27, 2017

Contributor

Modifications completed

@@ -119,15 +119,31 @@ void pin_mode(PinName pin, PinMode pupd)
switch(port_num) {
case PortA:
if(pupd != 0)

This comment has been minimized.

@0xc0170

0xc0170 Apr 26, 2017

Member

Use

if () {

}

as the rest of this file

This comment has been minimized.

@khj098765

khj098765 Apr 27, 2017

Contributor

Modifications completed

@@ -110,10 +119,11 @@ int gpio_irq_init(gpio_irq_t *obj, PinName pin, gpio_irq_handler handler, uint32
else
obj->irq_n = PORT3_IRQn;
//obj->event = EDGE_FALL;

This comment has been minimized.

@0xc0170

0xc0170 Apr 26, 2017

Member

please remove any "dead" code like this, same o nthe line 109

This comment has been minimized.

@khj098765

khj098765 Apr 27, 2017

Contributor

Modifications completed

port_generic_handler(GPIOA, 0);
test_tmp++;

This comment has been minimized.

@0xc0170

0xc0170 Apr 26, 2017

Member

test variable that should be removed?

This comment has been minimized.

@khj098765

khj098765 Apr 27, 2017

Contributor

Modifications completed

@@ -58,11 +58,11 @@ void analogin_init(analogin_t *obj, PinName pin)
obj->pin = pin;
// The ADC initialization is done once
if (adc_inited == 0) {
//if (adc_inited == 0) {

This comment has been minimized.

@0xc0170

0xc0170 Apr 26, 2017

Member

second ADC pin will reinit entire ADC? What is this change fixing? What was wrong with the previous version?

For future reference, It would be beneficial if any logical change would have separate commit, like this one, would be explained via commit message why and what has changed.

This comment has been minimized.

@khj098765

khj098765 Apr 27, 2017

Contributor

Problems occurred when Two or more pins were used...
So we modified this code.

@0xc0170 0xc0170 added the needs: work label Apr 26, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 26, 2017

WIZnet has done all the pretesting (Greentea + CI-test) and analysis already.

It would be good to share test results from mbed test run for these platforms that we are about to enable.

I left few comments, mostly style related.

Note, we are soon enough going to land new RTX2 to master.

khj098765 added some commits Apr 26, 2017

Modified coding style of mbed.
- deleted dead code
- deleted test code

@khj098765 khj098765 closed this Apr 26, 2017

@khj098765

This comment has been minimized.

Contributor

khj098765 commented Apr 26, 2017

modified this issues
we have already been shared to andrew about Greentea TEST & Ci TEST results.
have another problem?
Please tell us.

@khj098765 khj098765 reopened this Apr 26, 2017

@andrewc-arm

This comment has been minimized.

andrewc-arm commented Apr 27, 2017

It would be good to share test results from mbed test run for these platforms that we are about to enable.

This is the test result log. WIZnet_W7500_test_logs.zip
Please note that I explicitly asked WIZnet to remove RTC and PWM test items since those will be waivered and added to the board's disclaimer.

@@ -119,7 +114,6 @@ int gpio_irq_init(gpio_irq_t *obj, PinName pin, gpio_irq_handler handler, uint32
else
obj->irq_n = PORT3_IRQn;
obj->event = EDGE_NONE;

This comment has been minimized.

@0xc0170

0xc0170 Apr 27, 2017

Member

why was this event removed? it's a style commit, but this is functional change? or dead code not used?

This comment has been minimized.

@khj098765

khj098765 Apr 27, 2017

Contributor

Sorry, It is a mistake. I put it back.

@@ -141,10 +145,14 @@ void gpio_irq_set(gpio_irq_t *obj, gpio_irq_event event, uint32_t enable)
obj->rise_null = 0;
}
else if (event == IRQ_FALL) {
gpio->INTPOLSET &= ~obj->pin_index;
//gpio->INTPOLSET &= ~obj->pin_index;

This comment has been minimized.

@0xc0170

0xc0170 Apr 27, 2017

Member

still dead code in here?

This comment has been minimized.

@khj098765

khj098765 Apr 27, 2017

Contributor

Modifications completed

case PortA:
if(pupd != 0)

This comment has been minimized.

@0xc0170

0xc0170 Apr 27, 2017

Member

should be

if () {

}

This comment has been minimized.

@khj098765

khj098765 Apr 27, 2017

Contributor

Modifications completed

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 27, 2017

Thanks for sharing the test results, all fine!

PE team and WIZnet have agreed on waiver of RTC and PWM test.

What is with RTC and PWM? Out of curiosity.

I left 2 minor comments, once reviewed, I'll run CI.

@andrewc-arm

This comment has been minimized.

andrewc-arm commented Apr 27, 2017

RTC: Real Time Clock. For accurate time application like digital clock or constant blip.
PWM: Pulse Width Modulation

delete dead code. -> gpio_irq_api.c
modified coding style. -> pinmap.c
@khj098765

This comment has been minimized.

Contributor

khj098765 commented Apr 27, 2017

All edited.
Please merge pull request.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 27, 2017

RTC: Real Time Clock. For accurate time application like digital clock or constant blip.
PWM: Pulse Width Modulation

My question above was why RTC and PWM are waived - what will be added to the board page? The reason

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 27, 2017

/morph test

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Apr 27, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Apr 27, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

@@ -2460,7 +2460,7 @@
"supported_toolchains": ["uARM", "ARM"],
"inherits": ["Target"],
"device_has": ["ANALOGIN", "I2C", "INTERRUPTIN", "PORTIN", "PORTINOUT", "PORTOUT", "PWMOUT", "RTC", "SERIAL", "SPI", "SPISLAVE", "STDIO_MESSAGES"],
"release_versions": ["2"]
"release_versions": ["2", "5"]

This comment has been minimized.

@bridadan

bridadan Apr 27, 2017

Contributor

You must support ARM, GCC_ARM, and the IAR compilers to enable the release version "5" (same goes for the below changes). That's why the CI failed.

This comment has been minimized.

@andrewc-arm

andrewc-arm Apr 28, 2017

No... We don't have time to support GCC_ARM and IAR.
It supports ARM compiler only for now.
We have May 1st contest coming.
Let me call you and others internally.

This comment has been minimized.

@andrewc-arm

andrewc-arm Apr 28, 2017

I asked WIZnet to change the supported toolchains. Let's see how this goes.

This comment has been minimized.

@khj098765

khj098765 Apr 28, 2017

Contributor

Added support for GCC and IAR.

@andrewc-arm

This comment has been minimized.

andrewc-arm commented Apr 28, 2017

My question above was why RTC and PWM are waived - what will be added to the board page? The reason

RTC:

  • Currently WIZnet board's capacitance of crystal oscillator need change. Since RTC can be outsourced, PE team decided to put this waiver temporarily.

PWM:

  • WIZnet's PWM works. However, WIZnet's GPIO does not support Dual Edge Triggering. Theirs only support Single Edge Triggering. Our current PWM test is based on InterruptInt with rise() and fall() handling using Dual Edge Triggering. For this urgent time being, PE team decided to skip the PWM test and support later.

About the Feature Documentation:
Yes. There should be feature documentation warning this limitation in their board page. This is the candidate message.

  • Due to low need, Dual Edge Triggered GPIO will not be supported.
    • InterruptIn class’ rise() or fall() member function will override the previous callback setting. (ex. rise(A) -> fall(B): only fall(B) is active)
  • RTC will be temporarily unsupported and will be enabled later. (minor HW adjustment may be required)
  • PWM’s stability could not be tested for internal reasons.
@andrewc-arm

This comment has been minimized.

andrewc-arm commented Apr 28, 2017

Please ignore my comment about "urgency". I think we can relax and do this PR properly with time. :)

@khj098765

This comment has been minimized.

Contributor

khj098765 commented Apr 28, 2017

Added support for GCC and IAR.
completed?

@bridadan

This comment has been minimized.

Contributor

bridadan commented Apr 28, 2017

Awesome, thanks for adding that! I'll run the CI again.

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Apr 28, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 107

Build failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 28, 2017

@khj098765 From the Ci above, you can review the logs with errors. One of them:

```[Fatal Error] RTX_Conf_CM.c@54,0: [Pe035]: #error directive: "no target defined"```` . Can you please test ? I would run mbed test for RTOS tests, seems like the target is not getting in for RTX (some definitions missing?)

#define OS_CLOCK 20000000
#endif
#endif

This comment has been minimized.

@bridadan

bridadan Apr 28, 2017

Contributor

Build is failing because of this extra #endif I believe!

This comment has been minimized.

@andrewc-arm

andrewc-arm May 2, 2017

Yes. This should be moved to line 66.

This comment has been minimized.

@0xc0170

0xc0170 May 2, 2017

Member

@khj098765 Can you update?

This comment has been minimized.

@khj098765

khj098765 May 8, 2017

Contributor

I've fixed and updated this issue.

@andrewc-arm

This comment has been minimized.

andrewc-arm commented May 2, 2017

WIZnet is on vacation for this whole week. So they will be able to respond on next Monday.
Justin is the WIZnet contact person.

Justin, @khj098765, our team did the review of GCC_ARM and I got this comment. Please review.


Their startup code of GCC_ARM needs to call _start function in GCC runtime (then, the _start function calls main function).

https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_WIZNET/TARGET_W7500x/TARGET_WIZwiki_W7500/device/TOOLCHAIN_GCC_ARM/startup_W7500.S#L191

https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_WIZNET/TARGET_W7500x/TARGET_WIZwiki_W7500ECO/device/TOOLCHAIN_GCC_ARM/startup_W7500.S#L191

https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_WIZNET/TARGET_W7500x/TARGET_WIZwiki_W7500P/device/TOOLCHAIN_GCC_ARM/startup_W7500.S

/*bl    _start*/ This code code should comment in (enabled)
bl main             This should be removed

This is explained here:
http://stackoverflow.com/questions/29694564/what-is-the-use-of-start-in-c

And similar issue raised here:
#3622
#889 (comment)

Also, I recommend to remove *.o file to avoid multiple link errors.

https://github.com/ARMmbed/mbed-os/tree/master/targets/TARGET_WIZNET/TARGET_W7500x/TARGET_WIZwiki_W7500ECO/device/TOOLCHAIN_GCC_ARM

@0xc0170 0xc0170 added needs: work and removed needs: CI labels May 2, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 2, 2017

Could we get a better name for this? "modify the code to support" is always happening.

@khj098765

This comment has been minimized.

Contributor

khj098765 commented May 8, 2017

I'm sorry I keep repeating.
There will be no more errors...
thank you

@andrewc-arm

This comment has been minimized.

andrewc-arm commented May 8, 2017

@khj098765, Justin, could you please change the title to "[WIZnet][W7500*] Support for mbed OS 5"? Or you can improvise as you wish. :)

@andrewc-arm

This comment has been minimized.

andrewc-arm commented May 8, 2017

/morph test

@khj098765

This comment has been minimized.

Contributor

khj098765 commented May 8, 2017

Yes I can change title to "[WIZnet][W7500*] Support for mbed OS 5"

@khj098765 khj098765 changed the title from Modifying the code to support mbed OS 5 -> add mbed_rtx.h to [WIZnet][W7500*] Support for mbed OS 5 May 8, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 8, 2017

/morph test

ADC_Init();
}
// The ADC initialization
ADC_Init();

This comment has been minimized.

@0xc0170

0xc0170 May 8, 2017

Member

I believe I asked earlier but cant find my comment. Why are you removing this static init variable? If I have 2 pins, this will reinit ADC 2x (power down and up as I can see in this changeset)? Is this desirable behavior? what was wrong with code as it was?

This comment has been minimized.

@andrewc-arm

andrewc-arm May 10, 2017

I talked with WIZnet Justin and he said this

  • Previously it worked fine. They thought ADC_Init() can be done only once and that was how it was implemented.
  • During the CI ADC test, they found that previous ADC run had some 'lingering effect'.
  • Now they realized that ADC should be hardware reset, which is done by calling ADC_Init() all the time when actual init is happening.
  • This code is now better and more intuitive and correctly resets the ADC hardware.

This comment has been minimized.

@0xc0170

0xc0170 May 10, 2017

Member

Interesting, hw reset peripheral for each ADC pin. If this is their hw requirement to restart adc like that for each adc channel initialization, ok.

@mbed-bot

This comment has been minimized.

mbed-bot commented May 8, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 165

All builds and test passed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 9, 2017

@andrewc-arm happy with this patch as it is now?

@khj098765 Can you please answer ADC question above

@andrewc-arm

This comment has been minimized.

andrewc-arm commented May 10, 2017

@andrewc-arm happy with this patch as it is now?

Yes. But let me clear the ADC question you raised, I will talk to Justin why it was that way before and why it is this way now.

I just posted the reason for the ADC code change and I think this can be patched? :)

Resolved

@0xc0170 0xc0170 added ready for merge and removed needs: work labels May 10, 2017

@andrewc-arm

This comment has been minimized.

andrewc-arm commented May 12, 2017

Could you let us know when this will be merged to mbed-os repo? The WIZnet contest makers are waiting. :)

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 12, 2017

@andrewc-arm We will review this as a candidate for inclusion in mbed OS on Monday, May 15th. Sorry for the wait.

@0xc0170 0xc0170 merged commit 444ff09 into ARMmbed:master May 15, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment