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

Fix ethernet API build warnings for LPC4088 #5320

Merged
merged 1 commit into from Oct 30, 2017

Conversation

Projects
None yet
7 participants
@kegilbert
Contributor

kegilbert commented Oct 13, 2017

LPC4088 ethernet_api build warning fixes to address #5318

Looked like an issue with using a typedef on packed structs in the way it was originally written (https://stackoverflow.com/questions/12213866/is-attribute-packed-ignored-on-a-typedef-declaration).

Does this look reasonable? Tested on a locally exported blinky example for make_armc5. Saw the same warnings as the above issue before moving the PACKED attribute to the end of the struct declaration.

As a quick sanity check I ran the following with and without the PACKED attribute in the struct declaration to confirm the attribute was not silently dropped.

#define MBED_PACKED(struct) struct __attribute__((packed))
#define PACKED MBED_PACKED()

int main(void) {
    struct test {
        unsigned char a;
        unsigned short b;
        unsigned long c;
    } PACKED;

    typedef struct test tTest;

    tTest var1;
    printf("%d\r\n", sizeof(var1));
}

@kegilbert kegilbert requested review from 0xc0170 and sarahmarshy Oct 13, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 17, 2017

LGTM, lets run build only to be certain this is all toolchains compatible

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 19, 2017

/morph build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 19, 2017

@sarahmarshy review pls

One way to do this also is to use MBED_PACKED that we provide, would that eliminate these warnings?

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 19, 2017

Build : SUCCESS

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

Skipping test trigger, missing label 'NEED CI'

@kegilbert

This comment has been minimized.

Contributor

kegilbert commented Oct 19, 2017

Should probably switch to MBED_PACKED actually since it looks like PACKED is older, but it just defines back to MBED_PACKED internally:
https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_toolchain.h#L375

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 24, 2017

@sarahmarshy review pls

BUMP

@sarahmarshy

LGTM!

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 24, 2017

/morph test

@mbed-ci

This comment has been minimized.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 26, 2017

Should probably switch to MBED_PACKED actually since it looks like PACKED is older, but it just defines back to MBED_PACKED internally:
https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_toolchain.h#L375

@kegilbert Are you proposing to make a change still on this PR then ?

Replace PACKED attribute on lpc4088 ethernet structs with MBED_PACKED…
…. Placement of packed attribute was causing warnings due to following typedef

@kegilbert kegilbert force-pushed the kegilbert:fix-build-warnings-lpc4088 branch to 901157b Oct 26, 2017

@kegilbert

This comment has been minimized.

Contributor

kegilbert commented Oct 26, 2017

@adbridge I just updated the PR, I was going to gauge if anyone had thoughts on it first. Decided to change it to MBED_PACKED since I'd seen it used a bit more in mbed_toolchain and some filesystem implementations.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 26, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 26, 2017

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Oct 26, 2017

Please ignore the failure #5320 (comment) , plugin triggered the build twice.

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 26, 2017

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Oct 27, 2017

@theotherjimmy theotherjimmy merged commit 02f1d01 into ARMmbed:master Oct 30, 2017

5 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-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

@kegilbert kegilbert deleted the kegilbert:fix-build-warnings-lpc4088 branch Oct 30, 2017

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