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.

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

Add screen power on/off capability to web interface, refactorings #279

Merged
merged 3 commits into from
Oct 25, 2018

Conversation

rdumusc
Copy link

@rdumusc rdumusc commented Oct 22, 2018

No description provided.

@rdumusc
Copy link
Author

rdumusc commented Oct 22, 2018

Hold on; looks like some more changes are required to update the screen status icon after the screens have been turned on/off

@rdumusc rdumusc force-pushed the poweron branch 3 times, most recently from ce43645 to 198729a Compare October 24, 2018 09:08
@rdumusc
Copy link
Author

rdumusc commented Oct 24, 2018

OK, should be fine now (tested on 5th floor).


bool _getResult(const std::vector<bool>& results)
{
for (const auto result : results)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (const auto result : results)
return std::find(results.begin(), results.end(), false) == results.end();

Copy link
Author

Choose a reason for hiding this comment

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

thanks, done

{
ScreenState _getResult(const std::vector<ScreenState>& results)
{
const auto state = results[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

what if results empty?

Copy link
Author

Choose a reason for hiding this comment

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

Right, this function should be safe even though it normally doesn't happen in this context, updated.

namespace
{
const int serialTimeout = 1000; // in ms
const int powerStateTimer = 60000; // in ms
const int readTimeoutMs = 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const int readTimeoutMs = 1000;
constexpr int readTimeoutMs = 1000;

Copy link
Author

Choose a reason for hiding this comment

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

done

#ifndef ScreenController_H
#define ScreenController_H
#ifndef SCREEN_CONTROLLER_H
#define SCREEN_CONTROLLER_H
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer #pragma once

Copy link
Author

Choose a reason for hiding this comment

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

I agree that it's less work and less error prone, I was using pragma once before joining the BBP then I stopped to maintain a consistent style, and now I got used to doing it by hand... ;-) The only times I found hard-coded names to be useful is in the rare cases when you need fine control over which files are included when using conflicting or broken third-party libraries. But that's very uncommon and I agree that it does not apply to this project. Maybe I should consider doing a batch change at some point.

@rdumusc rdumusc merged commit 050e966 into BlueBrain:master Oct 25, 2018
@rdumusc rdumusc deleted the poweron branch October 25, 2018 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants