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

Update for VK_RZ_A1H #6245

Merged
merged 1 commit into from Mar 20, 2018

Conversation

Projects
None yet
7 participants
@mbedNoobNinja
Contributor

mbedNoobNinja commented Mar 1, 2018

Description

Platform VK_RZ_A1H synced with mbed-os 5.7.6

Test Rezults:
VK_RZ_A1H_ARM_TESTs.log
VK_RZ_A1H_GCC_TESTs.log
VK_RZ_A1H_IAR_TESTs.log

Pull request type

  • Fix
@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 1, 2018

@TomoYamanaka, if you could also take a look at this PR, that would be great.

@cmonr cmonr added the needs: review label Mar 1, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 1, 2018

@mbedNoobNinja It looks like there were a lot of changes that went into this PR. Could you clarify what you mean by "synced with mbed-os 5.7.6"?

@mbedNoobNinja

This comment has been minimized.

Contributor

mbedNoobNinja commented Mar 2, 2018

I mean after a bug was introduced in issue № (#5777), this is the supposed fix that should resolve that issue. In short the last working mbed-os for the platform VK_RZ_A1H was 5.4.7 and there was no Cortex A9 support then. But since the maintenance is already a fact with 5.6.0, and there were some drastic changes in folder's structure as whole (#4440) and so on, ... update was needed. Now VK_RZ_A1H is "aligned" with GR_PEACH (RZ_A1H) & GR_LYCHEE (RZ_A1LU) and VK_RZ_A1H should be compilable with 5.6.x and above.

@bulislaw

Could you extend the commit message, and describe what's actually going on. Something like in your last comment. Also you are adding release_version: [2, 5] to VK_RZ_A1H so that's a basically new board support as it was not active before.

@@ -31,6 +31,7 @@
#define TRANSACTION_QUEUE_SIZE_SPI 16

This comment has been minimized.

@bulislaw

bulislaw Mar 2, 2018

Member

Can you delete the unnecessary empty lines.

This comment has been minimized.

@mbedNoobNinja

mbedNoobNinja Mar 2, 2018

Contributor

Done.

@mbedNoobNinja mbedNoobNinja force-pushed the mbedNoobNinja:Sync_PR branch from 26318fc to a0af134 Mar 2, 2018

@0xc0170 0xc0170 added needs: work and removed needs: review labels Mar 5, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 5, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 5, 2018

@bulislaw Is the updated commit message a bit better?

@cmonr cmonr added needs: review and removed needs: work labels Mar 5, 2018

@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Mar 7, 2018

@TomoYamanaka, if you could also take a look at this PR, that would be great.

Sorry for late reply. I think that it is okay regarding the changed code.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 7, 2018

After I read the referenced issue above (#5777) that I created earlier, I understand the changes here. This target was disabled as it was out of sync with other Renesas targets. This PR is updating the codebase, aligning it with the rest of the targets and thus reenabling it back to 2 and 5 (thus that release version).

@mbedNoobNinja it's always good to describe briefly at least changes even if reference is provided (that came later ,thanks!). Can you amend you commit message to contain these details? What update is it -

I mean after a bug was introduced in issue № (#5777), this is the supposed fix that should resolve that issue. In short the last working mbed-os for the platform VK_RZ_A1H was 5.4.7 and there was no Cortex A9 support then. But since the maintenance is already a fact with 5.6.0, and there were some drastic changes in folder's structure as whole (#4440) and so on, ... update was needed. Now VK_RZ_A1H is "aligned" with GR_PEACH (RZ_A1H) & GR_LYCHEE (RZ_A1LU) and VK_RZ_A1H should be compilable with 5.6.x and above.

This info is crucial here

@0xc0170 0xc0170 removed the request for review from hanno-arm Mar 7, 2018

@mbedNoobNinja mbedNoobNinja force-pushed the mbedNoobNinja:Sync_PR branch from a0af134 to 1226d06 Mar 8, 2018

@mbedNoobNinja

This comment has been minimized.

Contributor

mbedNoobNinja commented Mar 8, 2018

Yeah. The commit message became a little longer ...

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 8, 2018

Yeah. The commit message became a little longer ...

That is why there is a commit message, can be few paragraphs long if it needs to be. I find it now more clear (note that first line should be the most 50 characters, as you can see how it wraps here).

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 8, 2018

@mbedNoobNinja Looking at the test results, some of the networking tests fail? Can you please clarify.

@mbedNoobNinja

This comment has been minimized.

Contributor

mbedNoobNinja commented Mar 8, 2018

Yes, the Ethernet functions very well, ( at least the sockets -> I managed to receive video RTSP stream on mbed through both: UDP & TCP) even though these 3 tests

mbed-os-tests-netsocket-socket_sigio
mbed-os-tests-netsocket-tcp_hello_world
mbed-os-tests-netsocket-udp_dtls_handshake

always fails, and I don't know why. Even in OS 5.4.7 TCP hello world has never passed.
The strange thing is even in RZ_A1H UDP DTLS handshake also does not pass, but sigio & TCP hello world pass.

Perhaps this is an Internet provider related issue, I don't know.
In 5.4.7 TCP hello world failed because of timeout (the response of the request arrived in two pieces and only the first was checked and therefore - incomplete response)
For the situation here I have no idea.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 8, 2018

@TomoYamanaka Are you aware of these issues, are they reproducible ? we shall take them to a separate issue.

This PR does not touch networking code. Shall ethernet be disabled until it's fixed?

@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Mar 9, 2018

@0xc0170
I have never faced the above NET test. Whether Mbed OS 5.4 or Mbed OS 5.7, GR-PEACH and GR-LYCHEE passed the test.
It may be a problem with @mbedNoobNinja 's Internet environment.

Addressed via new commit

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 9, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 9, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 9, 2018

@mbedNoobNinja Please review the ARMCC build problem

@0xc0170 0xc0170 added needs: work and removed needs: review labels Mar 9, 2018

@mbedNoobNinja

This comment has been minimized.

Contributor

mbedNoobNinja commented Mar 9, 2018

Hm strange. With mbed CLI it is compiled successfully (blinky example).
When mbed_config.h is created and where is located from test environment point of view ?
Is it creates temporary linker script (.link_script.sct) ?
If Yes -> then the beginning of this file is it starts with #! armcc -E --cpu=Cortex-A9-I mbed-os\targets\TARGET_RENESAS\TARGET_RZ_A1XX\TARGET_VK_RZ_A1H\device\TOOLCHAIN_ARM_STD
I mean the path where mbed_config.h is located, should be included in the .link_script file as well.
With mbed CLI apparently there is no problem because mbed_config.h & .link_script.sct are in the same folder and there is no need to be explicitly included.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 12, 2018

I can not reproduce the problem. I fetched your branch locally, build the sermaphore test without any error (I am using locally armcc 5.06 update 5 at the moment)

@studavekar Can you reproduce the build failure here?

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 12, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 12, 2018

Locally mbed test --compile -m VK_RZ_A1H -t ARM -DMBED_HEAP_STATS_ENABLED=1 -DMBED_STACK_STATS_ENABLED=1 -DMBED_TRAP_ERRORS_ENABLED=1 --clean - no failure 😕

Enabled os5 support for VK_RZ_A1H & synced with rest Renesas targets !
Mbed-os 5.4.7 was the last unofficial working support for this target.
Since Mbed-os 5.6.0, the support is now official and VK_RZ_A1H is now "codebase aligned" with GR_PEACH (RZ_A1H) & GR_LYCHEE (RZ_A1LU) !

@mbedNoobNinja mbedNoobNinja force-pushed the mbedNoobNinja:Sync_PR branch from 1226d06 to cf8fd20 Mar 20, 2018

@mbedNoobNinja

This comment has been minimized.

Contributor

mbedNoobNinja commented Mar 20, 2018

Found the problem --preinclude does not act on h files, only on c Now it should compile. Hit a build 👍 when it's convenient.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 20, 2018

Found the problem --preinclude does not act on h files, only on c Now it should compile. Hit a build 👍 when it's convenient.

🙌

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 20, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Mar 20, 2018

/morph mbed2-build

@cmonr cmonr added ready for merge and removed needs: work labels Mar 20, 2018

@cmonr

cmonr approved these changes Mar 20, 2018 edited

All prior comments were addressed, and looks like the only change outside of TARGET_RENESAS was targets.json.

👍

@cmonr cmonr merged commit 2c7f909 into ARMmbed:master Mar 20, 2018

11 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Passed, code size is 10060B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 20, 2018

@mbedNoobNinja mbedNoobNinja deleted the mbedNoobNinja:Sync_PR branch Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment