Skip to content
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.

Sign up for GitHub

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

Jump to bottom

msp430: no optimization fix #5188

Closed
wants to merge 3 commits into from
Closed

msp430: no optimization fix #5188

wants to merge 3 commits into from

Conversation

cgundogan
Copy link
Member

addresses #4954, but does not fix it.
I modified all functions so that the compiler doesn't complain anymore.
However, if I flash the telosb board with the default example, then I can see RIOT's greeting, but it does not respond to my input. When I flash the hello-world example, then I get a boot-loop.

Necessary to test this PR: in cpu/Makefile.include.msp430_common change CFLAGS_OPT to O0.

@cgundogan cgundogan added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: MSP Platform: This PR/issue effects MSP-based platforms State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Mar 29, 2016
@cgundogan
Copy link
Member Author

@OlegHahm I assigned you as a placeholder, and because you authored many of the files in question.

@Kijewski could you also have a closer look at the changes?

@jnohlgard
Copy link
Member

I am not very familiar with msp430 so I won't be of much help, but did you examine the assembly output of the compiler?

@miri64 miri64 modified the milestone: Release 2016.07 Mar 29, 2016
@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 29, 2016
thomaseichinger
thomaseichinger reviewed Mar 29, 2016
View reviewed changes
cpu/cc430/periph/timer.c
@@ -135,7 +136,8 @@ ISR(TIMER_ISR_CC0, isr_timer_a_cc0)
__exit_isr();
}

ISR(TIMER_ISR_CCX, isr_timer_a_ccx_isr)
void __attribute__((optimize("omit-frame-pointer"), no_instrument_function,
interrupt (TIMER_ISR_CCX))) isr_timer_a_ccx_isr(void)
Copy link
Member

Choose a reason for hiding this comment

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

@cgundogan Is it on purpose this ISR is not naked?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, otherwise the compiler would give messages like described #4954

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 31, 2016
@miri64 miri64 mentioned this pull request Apr 20, 2016
Closed
@kYc0o
Copy link
Contributor

kYc0o commented Jul 12, 2016

news on this one?

@kYc0o
Copy link
Contributor

kYc0o commented Jul 22, 2016

@cgundogan can you make it for this release?

@kYc0o kYc0o modified the milestones: Release 2016.10, Release 2016.07 Jul 25, 2016
@miri64
Copy link
Member

miri64 commented Nov 4, 2016

ping?

@kYc0o
Copy link
Contributor

kYc0o commented Nov 6, 2016

Needs rebase BTW...

@OlegHahm
Copy link
Member

OlegHahm commented Nov 7, 2016

Needs rebase BTW...

Are you sure? Anyway, it would be in vain since MSP430 still seems to break with this PR and -O0.

@kYc0o
Copy link
Contributor

kYc0o commented Nov 8, 2016

The rebase was only to update the PR and get rid of travis, but you're right, is not really needed.

@miri64
Copy link
Member

miri64 commented Nov 9, 2016

Given the age of the both this PR and the last build a rebuild might be a good idea. I agree with @kYc0o on that.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 9, 2016
@miri64
Copy link
Member

miri64 commented Nov 9, 2016

Postponed in correspondence with the author.

@miri64 miri64 added this to the Release 2017.01 milestone Nov 9, 2016
@miri64 miri64 removed this from the Release 2016.10 milestone Nov 9, 2016
@OlegHahm
Copy link
Member

OlegHahm commented Nov 9, 2016

Given the age of the both this PR and the last build a rebuild might be a good idea. I agree with @kYc0o on that.

I merged it locally. It still fails. So, from my perspective a rebase seems like a waste of time.

@kYc0o kYc0o mentioned this pull request Jan 12, 2017
Closed
@kYc0o
Copy link
Contributor

kYc0o commented Jan 24, 2017

So this PR is still needed? It is compatible with #6445 ?

@PeterKietzmann PeterKietzmann modified the milestones: Release 2017.01, Release 2017.04 Jan 26, 2017
@smlng
Copy link
Member

smlng commented Dec 15, 2017

@cgundogan any thoughts on this one - like close as memo?

@smlng smlng removed this from the Release 2018.01 milestone Jan 12, 2018
@miri64
Copy link
Member

miri64 commented Feb 15, 2018

No significant progress for over a year. Closing as memo for now.

@miri64 miri64 closed this Feb 15, 2018
@miri64 miri64 added the State: archived State: The PR has been archived for possible future re-adaptation label Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@OlegHahm OlegHahm

Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: MSP Platform: This PR/issue effects MSP-based platforms State: archived State: The PR has been archived for possible future re-adaptation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@cgundogan @jnohlgard @kYc0o @miri64 @OlegHahm @smlng @thomaseichinger @kaspar030 @PeterKietzmann