Skip to content

cpu/samd21: set optimization level to -Os#2848

Merged
kaspar030 merged 1 commit intoRIOT-OS:masterfrom
haukepetersen:fix_samr21_optimization
May 29, 2015
Merged

cpu/samd21: set optimization level to -Os#2848
kaspar030 merged 1 commit intoRIOT-OS:masterfrom
haukepetersen:fix_samr21_optimization

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Apr 21, 2015

I was wondering about the huge code-size for the samr21-xpro - turned out, the optimization for the board is turned off... Looks better now.

@haukepetersen haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Apr 21, 2015
@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Apr 22, 2015

Nice one! ACK, waiting for Travis..

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Apr 22, 2015

Have you tested with some applications to check if this has no unexpected side effect?

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Apr 22, 2015

We did, it has some problems with the GPIO implementation AFAIR.

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Apr 22, 2015

Tested with some easy applications but not with peripheral tests. In this case I revert my ACK and need to do some further testings :(

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Apr 22, 2015

I did some more tests, the GPIO drivers are not cleanly implemented. I was not really going to look into this in detail... But I will try to do so.

Just a remark: the GPIO issues are actually unrelated to this PR, it just seems that enabling the compiler optimization triggers the compiler to detect them...

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Apr 22, 2015

Just a remark: the GPIO issues are actually unrelated to this PR, it just seems that enabling the compiler optimization triggers the compiler to detect them...

Well, while I think it's okay for this particular test application, that was exactly the idea behind my question: even if the underlying problem is unrelated to this change, it may introduce failures that won't happen without the optimization. rpl_udp is typically a good candidate, but I have only one SAMR21 here.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Apr 22, 2015

I will look into this once I find some time...

@kaspar030 kaspar030 mentioned this pull request Apr 22, 2015
Merged
21 tasks
@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Apr 28, 2015

I think we can either (i) merge this PR and open an issue, close this and open an issue (iii) close this PR as it is also part of #2852 and move the discussion there

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Apr 28, 2015

I say lets leave this PR open for now and and see if after #2865 is merged either this PR or #2852 is faster...

@OlegHahm OlegHahm modified the milestone: Release 2015.06 Apr 29, 2015
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Apr 30, 2015

Just saw that the GPIO warnings are not the only issue for the samr21-xpro board. When enabling -Os, the tests/driver_at86rf2xx application does also not work -> it returns an error in the radio initialization function. I would suspect something the low level drivers to cause that...

@biboc
Copy link
Copy Markdown
Member

biboc commented Apr 30, 2015

@haukepetersen, might be the cause of the GPIO as well? or SPI?

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented May 5, 2015

I had no time to look further into this, yet. But I hope to get to it in the end of the week.

@biboc biboc added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label May 26, 2015
@haukepetersen haukepetersen force-pushed the fix_samr21_optimization branch from fb42b72 to 32410bb Compare May 29, 2015 10:15
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented May 29, 2015

Good news: the samr21 is now doing fine with -Os. So please verify this and let's merge this soon to save some time flashing the nodes!

@haukepetersen haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 29, 2015
@haukepetersen haukepetersen changed the title boards/samr21-xpro: set optimization level to -Os cpu/samd21: set optimization level to -Os May 29, 2015
biboc
biboc reviewed May 29, 2015
View reviewed changes
cpu/samd21/Makefile.include
Copy link
Copy Markdown
Member

@biboc biboc May 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we change it by -Os then?

Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 May 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the common makefile defines CFLAGS_OPT ?= -Os.

Copy link
Copy Markdown
Member

@biboc biboc May 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented May 29, 2015

somebody up to acking this?

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented May 29, 2015

Just connected the samr21, sec

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented May 29, 2015

works. ACK&go.

kaspar030 added a commit that referenced this pull request May 29, 2015
@kaspar030
Merge pull request #2848 from haukepetersen/fix_samr21_optimization
9ecaea4 
cpu/samd21: remove -O0 override
@kaspar030 kaspar030 merged commit 9ecaea4 into RIOT-OS:master May 29, 2015
@haukepetersen haukepetersen deleted the fix_samr21_optimization branch May 29, 2015 15:01
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented May 29, 2015

awesome, thx.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented May 29, 2015

seems we are moving in the right direction with this board :-)

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented May 29, 2015

yeah ;)

@PeterKietzmann PeterKietzmann mentioned this pull request May 24, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@PeterKietzmann PeterKietzmann

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2015.09

Development

Successfully merging this pull request may close these issues.

5 participants

@haukepetersen @PeterKietzmann @OlegHahm @kaspar030 @biboc