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

Remove redundant ISR test. #5326

Merged
merged 1 commit into from Oct 20, 2017

Conversation

Projects
None yet
8 participants
@mprse
Member

mprse commented Oct 17, 2017

Description

As a result of discussion with @bulislaw we agreed that ISR test is redundant and can be removed since ticker test provides scenario (#5233) which proves that interrupts are correctly handled:

  • ticker ISR is executed in the expected point of time and it is executed only once.

Status

READY

Migrations

NO

Related PRs

ARMmbed:feature-hal-spec-ticker | #5233

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 18, 2017

I restarted jenkins CI (not relevant failure there).

@mprse Once approved this will be tested. Is the destination branch up to date (expected to have CI green!) ?

@mprse

This comment has been minimized.

Member

mprse commented Oct 18, 2017

I restarted jenkins CI (not relevant failure there).

@mprse Once approved this will be tested. Is the destination branch up to date (expected to have CI green!) ?

Honestly I don't know how to check if this branch (ARMmbed:feature-hal-spec-ticker) is up to date and what does it mean. I have created a few PRs to this branch which are related to new ticker implementation and testing. Could you please provide more information what you expect me to do?

@tommikas

This comment has been minimized.

Contributor

tommikas commented Oct 19, 2017

@0xc0170

Is the destination branch up to date (expected to have CI green!) ?

I'm pretty sure it's out of date. jenkins/pr-head is failing on a build that requires mbed OS 5.6.1 or newer.

Specifically it's failing with IAR on this line:
extern SDBlockDevice sd(MBED_CONF_SD_SPI_MOSI, MBED_CONF_SD_SPI_MISO, MBED_CONF_SD_SPI_CLK, MBED_CONF_SD_SPI_CS);

with error message:
Error[Pe322]: object of abstract class type "SDBlockDevice" is not allowed:
pure virtual function "BlockDevice::erase" has no overrider
pure virtual function "BlockDevice::get_erase_size" has no overrider

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 19, 2017

@tommikas The destination branch was fast forwarded yesterday. It should be up to date and green. I restarted the job. Does jenkins CI merge with master (I cant locate the command in the logs).

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 19, 2017

/morph build

@tommikas

This comment has been minimized.

Contributor

tommikas commented Oct 19, 2017

Does jenkins CI merge with master (I cant locate the command in the logs).

It does. The pr head is passed as a pull/<change ID>/head reference which is then used to do the merge like this:

git fetch pull/5326/head
git checkout origin/feature-hal-spec-ticker
git merge FETCH_HEAD

It's done the same way both in the main os-cliapp build as well as the failing sub-build. Just verified that too. I also noticed that it's actually not just IAR that is failing. gcc and armcc failed too.

I'm seeing this warning in the logs as well: https://github.com/ARMmbed/sd-driver/blob/master/SDBlockDevice.cpp#L156

It's possible that Jenkins missed the notification from the latest push. Lets see what the current build says.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 19, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 19, 2017

Build : SUCCESS

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

Skipping test trigger, missing label 'NEED CI'

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Oct 19, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 19, 2017

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

@adbridge adbridge merged commit 07e2819 into ARMmbed:feature-hal-spec-ticker Oct 20, 2017

3 of 5 checks passed

ci-morph-build build in progress
Details
ci-morph-test
Details
AWS-CI uVisor Build & Test Success
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 20, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 23, 2017

Shouldn't this wait until #5233 is merged?

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 23, 2017

Ideally, but it doesn't really matter that much. Let's not revert it.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 23, 2017

This was pulled into the release-candidate branch since it has the tag release-version: 5.6.3, but it isn't on master. This should either be removed from the release-candidate branch or moved into master.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 23, 2017

CC @0xc0170 @adbridge

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 23, 2017

I'll create a new patch now

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 24, 2017

Arggh completely missed the fact this went to a feature branch! It could have just been removed from the release-candidate though....

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