-
Notifications
You must be signed in to change notification settings - Fork 5
Move OCC poll processing to occ_control #4
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
base: 1210
Are you sure you want to change the base?
Conversation
This is so we can make changes to the OCC poll response without kernel updates. Signed-off-by: Sheldon Bailey <baileysh@us.ibm.com>
cjcain
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass review
occ_manager.cpp
Outdated
| setSensorValueToNaN(instance); | ||
| for (auto& obj : statusObjects) | ||
| { | ||
| // int instance = obj->getOccInstanceID(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop is setting all OCCs sensors to NAN vs just the single instance that went away. I assume you need to add the check that was in the function you removed.
occ_status.cpp
Outdated
| { | ||
| for (const auto& [sensorPath, occId] : existingSensors) | ||
| { | ||
| if (occId == instance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the sensors stored in the status object (meaning you don't need to check the instance ID)?
They used to all be in the manager, but I assume they could move to status object (since I dont think any are shared between occs)
occ_status.hpp
Outdated
| return device.master(); | ||
| } | ||
|
|
||
| /** @brief Read OCC state (will trigger kernel to poll the OCC) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update comment
| capHardMin = PollRspHardMin; | ||
| capMax = PollRspMaxPower; | ||
|
|
||
| return parmsChanged; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment that this function is only called once when the master OCC goes active so it always returns that the parms have changed. (could get rid of the parmsChanged var as well)
This is so we can make changes to the OCC poll response without kernel updates. Signed-off-by: Sheldon Bailey <baileysh@us.ibm.com>
| obj->setSensorValueToNaN(); | ||
| if (instance == obj->getOccInstanceID()) | ||
| { | ||
| obj->setSensorValueToNaN(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could break out since there is only a single instance
| return; | ||
| }//end Kernel HandlePollAction | ||
|
|
||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge droppings
| } //end OCCC pollReadPcapBounds | ||
|
|
||
|
|
||
| #else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could put these in 2 different files? It is kind of confusing with both functions in the same file (when searching, etc)
maybe occ_poll_handler_kernel.cpp ???
Did you give up on having a poll handler base class, and then have app / kernel objects? or this is just first step?
Class OccPollHandler
{
common vars/functions
}
Class AppPoll : public OccPollHandler
{
app poll specific vars/functions
}
Class KernelPoll : public OccPollHandler
{
kernel poll specific vars/functions
}
// Then use with:
OccPollHandler *pollHandler;
if (kernel)
{
KernelPoll kPoll;
pollHandler = &kPoll;
}
else
{
AppPoll aPoll;
pollHandler = &aPoll;
}
pollHandler->pollReadStateStatus();
...
This moves the OCC poll from kernel to the occ-control app for more flexability.