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

core: allow DEBUG in mutex.c to run without DEVELHELP #5270

Merged
merged 2 commits into from
Apr 20, 2016
Merged

core: allow DEBUG in mutex.c to run without DEVELHELP #5270

merged 2 commits into from
Apr 20, 2016

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Apr 8, 2016

Tried to debug something without DEVELHELP and found that mutex.c isn't able to run without it. So I replaced thread names with PIDs.

@miri64 miri64 added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Apr 8, 2016
@miri64 miri64 modified the milestones: Release 2016.07, Release 2016.04 Apr 8, 2016
@miri64
Copy link
Member Author

miri64 commented Apr 8, 2016

Could go into this release IMHO (since it is more a bug fix than a feature).

@OlegHahm
Copy link
Member

OlegHahm commented Apr 8, 2016

Makes sense.

@OlegHahm OlegHahm added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 8, 2016
@miri64
Copy link
Member Author

miri64 commented Apr 8, 2016

Murdock is happy, but an ACK is missing.

@miri64
Copy link
Member Author

miri64 commented Apr 8, 2016

(two ACKs?)

@miri64 miri64 assigned kaspar030 and unassigned OlegHahm Apr 9, 2016
Yonezawa-T2
Yonezawa-T2 reviewed Apr 11, 2016
View reviewed changes
core/mutex.c
@@ -40,18 +40,18 @@
int _mutex_lock(mutex_t *mutex, int blocking)
{
unsigned irqstate = irq_disable();
DEBUG("%s: Mutex in use.\n", sched_active_thread->name);
DEBUG("PID[%u]: Mutex in use.\n", sched_active_thread->pid);
Copy link
Contributor

Choose a reason for hiding this comment

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

PRIkernel_pid?

@miri64
Copy link
Member Author

miri64 commented Apr 11, 2016

Addressed comment and unified a few other things.

@haukepetersen haukepetersen added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Apr 11, 2016
@haukepetersen
Copy link
Contributor

ACK, please squash

@haukepetersen
Copy link
Contributor

@authmillenon: please squash.

@kaspar030, @OlegHahm: one of you up for a 2nd ack?

OlegHahm
OlegHahm reviewed Apr 14, 2016
View reviewed changes
core/mutex.c
@@ -107,7 +107,7 @@ void mutex_unlock(mutex_t *mutex)

void mutex_unlock_and_sleep(mutex_t *mutex)
{
DEBUG("%s: unlocking mutex. queue.next: 0x%08x pid: %" PRIkernel_pid ", and taking a nap\n", sched_active_thread->name, (unsigned)mutex->queue.next, sched_active_pid);
DEBUG("PID[%" PRIkernel_pid "]: unlocking mutex. queue.next: 0x%08x, and taking a nap\n", sched_active_pid, (unsigned)mutex->queue.next);
Copy link
Member

Choose a reason for hiding this comment

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

While you're at it, you could fix the length of this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@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: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 17, 2016
@miri64
Copy link
Member Author

miri64 commented Apr 17, 2016

Squashed and addressed comments. While I was at it, I piggy-backed some code-style fixes

@miri64 miri64 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Apr 17, 2016
haukepetersen
haukepetersen reviewed Apr 18, 2016
View reviewed changes
core/mutex.c
thread_t *me = (thread_t*) sched_active_thread;
DEBUG("%s: Adding node to mutex queue: prio: %" PRIu32 "\n", me->name, (uint32_t)me->priority);
thread_t *me = (thread_t *)sched_active_thread;
DEBUG("PID[%" PRIkernel_pid "]: Adding node to mutex queue: prio: %" PRIu32 "\n", sched_active_pid, (uint32_t)me->priority);
Copy link
Contributor

Choose a reason for hiding this comment

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

if you fix the line length with this PR, you might as well fix it for all DEBUG statements?!

@haukepetersen
Copy link
Contributor

re-ACK if the one comment is adderssed.

@OlegHahm @kaspar030 ping

@miri64
Copy link
Member Author

miri64 commented Apr 18, 2016

Addressed comment and squashed immediately.

OlegHahm
OlegHahm reviewed Apr 18, 2016
View reviewed changes
core/mutex.c
@@ -35,34 +35,38 @@
#define ENABLE_DEBUG (0)
#include "debug.h"

#define MUTEX_LOCKED ((void*)-1)
#define MUTEX_LOCKED ((void *)-1)
Copy link
Member

Choose a reason for hiding this comment

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

unrelated (and unnecessary)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed, but don't agree... Pointers should be spaced according to the CC, so why not their casts?

@OlegHahm
Copy link
Member

Apart from the - IMO superfluous - white spaces: ACK

@kaspar030
Copy link
Contributor

I piggy-backed some code-style fixes

As they are debatable, please remove those.

@miri64
Copy link
Member Author

miri64 commented Apr 18, 2016

@kaspar030 where is the debate? I just see encouragements to shorten the lines....

@miri64
Copy link
Member Author

miri64 commented Apr 18, 2016

Uglified casts ;-), squashed immediately.

@haukepetersen
Copy link
Contributor

re-ACK

@haukepetersen
Copy link
Contributor

CI is green -> go

@haukepetersen haukepetersen merged commit f977654 into RIOT-OS:master Apr 20, 2016
@haukepetersen
Copy link
Contributor

backport needed

@miri64 miri64 deleted the core/fix/mutex-debug branch April 20, 2016 13:07
@miri64 miri64 mentioned this pull request Apr 20, 2016
Merged
@miri64 miri64 removed Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 20, 2016
@miri64
Copy link
Member Author

miri64 commented Apr 20, 2016

Murdock builds it again (I think). So I remove the label.

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

@kaspar030 kaspar030

Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2016.04
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@miri64 @OlegHahm @haukepetersen @kaspar030 @Yonezawa-T2