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

stdio: remove superfluous mutex_lock in init #3859

Merged
merged 1 commit into from
Oct 31, 2015
Merged

stdio: remove superfluous mutex_lock in init #3859

merged 1 commit into from
Oct 31, 2015

Conversation

OlegHahm
Copy link
Member

Stumbled across this while trying to debug mutexes - which are used inside this module...

@OlegHahm OlegHahm added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Sep 16, 2015
@OlegHahm OlegHahm added this to the Release 2015.09 milestone Sep 16, 2015
@haukepetersen
Copy link
Contributor

NACK. IMHO this adds runtime overhead to a very often used functionality - just so debugging for some core module works...

@kaspar030
Copy link
Contributor

agreed...

@OlegHahm
Copy link
Member Author

So, we need a printk?

@OlegHahm OlegHahm modified the milestones: Release NEXT MAJOR, Release 2015.09 Sep 16, 2015
@OlegHahm
Copy link
Member Author

btw. I assume that accessing the UART slow things down much more than checking a single value.

@haukepetersen
Copy link
Contributor

So, we need a printk?

I say no. All you can not debug using printf is mutexes and the peripheral UART driver. So introduce printk just for debugging mutexes? No...

@kaspar030
Copy link
Contributor

@OlegHahm move the mutex_lock below uart_init?

@OlegHahm
Copy link
Member Author

@kaspar030, seems legit. Was probably too late yesterday to see the simple solution.

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

looks better :-)

@kaspar030
Copy link
Contributor

@OlegHahm would you mind adding a short explanation to the commit?

@OlegHahm
Copy link
Member Author

@kaspar030, additional to "Otherwise one cannot enable debugging for mutexes."?

@kaspar030
Copy link
Contributor

needs rebase

@OlegHahm OlegHahm force-pushed the uart_stdio_uninitialized branch from 7c58f51 to 32f07da Compare September 29, 2015 10:44
@OlegHahm OlegHahm removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 29, 2015
@OlegHahm
Copy link
Member Author

rebased and squashed

@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 Sep 29, 2015
@kaspar030
Copy link
Contributor

Wait a minute. With the new ringbuffer, the mutex_lock() call is not needed anymore. We can drop it completely. Would you mind changing the PR accordingly?

edit I mean, just the call within uart_stdio_init().

@OlegHahm
Copy link
Member Author

So, just dropping the mutex_lock() in init?

@kaspar030
Copy link
Contributor

I don't have a board at hand, but if you could do a quick test and this doesn't break in a completly unforseen way, ACK.

@OlegHahm OlegHahm force-pushed the uart_stdio_uninitialized branch from 2ee9fa9 to 7b186a4 Compare October 31, 2015 08:34
@OlegHahm
Copy link
Member Author

rebased

@kaspar030
Copy link
Contributor

Would you mind changing the commit & PR descriptions? They don't reflect what's going on anymore. Other than that, ACK.

@OlegHahm
uart_stdio:mutex_lock is no longer needed for init
ee8aae7 
...since the introduction of the new ringbuffer.
@OlegHahm OlegHahm force-pushed the uart_stdio_uninitialized branch from 7b186a4 to ee8aae7 Compare October 31, 2015 10:55
@OlegHahm OlegHahm changed the title stdio: assure that uart isn't used uninitialized stdio: remove superfluous mutex_lock in init Oct 31, 2015
@OlegHahm
Copy link
Member Author

Can't deny that. Updated.

@kaspar030
Copy link
Contributor

Nice. Go!

kaspar030 added a commit that referenced this pull request Oct 31, 2015
@kaspar030
Merge pull request #3859 from OlegHahm/uart_stdio_uninitialized
3c5a36f 
stdio: remove superfluous mutex_lock in init
@kaspar030 kaspar030 merged commit 3c5a36f into RIOT-OS:master Oct 31, 2015
@OlegHahm OlegHahm deleted the uart_stdio_uninitialized branch October 31, 2015 15:05
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
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2015.12
Development

Successfully merging this pull request may close these issues.
3 participants
@OlegHahm @haukepetersen @kaspar030