manager: refactor setGroupState
setGroupState used to rely on std::set operations with non-obvious
workings involving std::inserter with custom comparators.
However the logic required seem to be simpler, therefore replacing the
set-based implementation with a map-based one.
The current state of the leds is now stored in a map, replacing the
previous two class members required with only one.
Change-Id: I3a812da4e5632c8258c45511f05ea1966b2f7208
Signed-off-by: Alexander Hansen <alexander.hansen@9elements.com>
diff --git a/manager/manager.cpp b/manager/manager.cpp
index 5a4c0ef..ec02419 100644
--- a/manager/manager.cpp
+++ b/manager/manager.cpp
@@ -14,6 +14,46 @@
namespace led
{
+// apply the led action to the map
+static void applyGroupAction(std::map<LedName, Layout::LedAction>& newState,
+ Layout::LedAction action)
+{
+ if (!newState.contains(action.name))
+ {
+ newState[action.name] = action;
+ return;
+ }
+
+ auto currentAction = newState[action.name];
+
+ if (currentAction.action == action.priority)
+ {
+ // if the current action is already the priority action,
+ // we cannot override it
+ return;
+ }
+
+ newState[action.name] = action;
+}
+
+// create the resulting new map from all currently asserted groups
+auto Manager::getNewMap(std::set<const ActionSet*> assertedGroups)
+ -> std::map<LedName, Layout::LedAction>
+{
+ std::map<LedName, Layout::LedAction> newState;
+
+ // update the new map with the desired state
+ for (const auto it : assertedGroups)
+ {
+ // apply all led actions of that group to the map
+ for (auto action : *it)
+ {
+ applyGroupAction(newState, action);
+ }
+ }
+ return newState;
+}
+
// Assert -or- De-assert
bool Manager::setGroupState(const std::string& path, bool assert,
ActionSet& ledsAssert, ActionSet& ledsDeAssert)
@@ -30,67 +70,35 @@
}
}
- // This will contain the union of what's already in the asserted ActionSet
- ActionSet desiredState{};
- for (const auto& grp : assertedGroups)
+ // create the new map from the asserted groups
+ auto newState = getNewMap(assertedGroups);
+
+ // the ledsAssert are those that are in the new map and change state
+ // + those in the new map and not in the old map
+ for (const auto& [name, action] : newState)
{
- desiredState.insert(grp->cbegin(), grp->cend());
+ if (ledStateMap.contains(name))
+ {
+ // check if the led action has changed
+ auto& currentAction = ledStateMap[name];
+
+ if (currentAction.action == action.action)
+ continue;
+ }
+
+ ledsAssert.insert(action);
}
- // Find difference between Combined and Desired to identify
- // which LEDs are getting altered
- ActionSet transient{};
- std::set_difference(combinedState.begin(), combinedState.end(),
- desiredState.begin(), desiredState.end(),
- std::inserter(transient, transient.begin()), ledComp);
- if (transient.size())
+ // the ledsDeAssert are those in the old map but not in the new map
+ for (const auto& [name, action] : ledStateMap)
{
- // Find common LEDs between transient and Desired to know if some LEDs
- // are changing state and not really getting DeAsserted
- ActionSet ledsTransient{};
- std::set_intersection(
- transient.begin(), transient.end(), desiredState.begin(),
- desiredState.end(),
- std::inserter(ledsTransient, ledsTransient.begin()), ledLess);
-
- // Find difference between above 2 to identify those LEDs which are
- // really getting DeAsserted
- std::set_difference(transient.begin(), transient.end(),
- ledsTransient.begin(), ledsTransient.end(),
- std::inserter(ledsDeAssert, ledsDeAssert.begin()),
- ledLess);
-
- // Remove the elements from Current that are being DeAsserted.
- // Power off LEDs that are to be really DeAsserted
- for (auto& it : ledsDeAssert)
+ if (!newState.contains(name))
{
- // Update LEDs in "physically asserted" set by removing those
- // LEDs which are De-Asserted
- auto found = currentState.find(it);
- if (found != currentState.end())
- {
- currentState.erase(found);
- }
+ ledsDeAssert.insert(action);
}
}
- // Now LEDs that are to be Asserted. These could either be fresh asserts
- // -or- change between [On]<-->[Blink]
- ActionSet temp{};
- std::unique_copy(desiredState.begin(), desiredState.end(),
- std::inserter(temp, temp.begin()), ledEqual);
- if (temp.size())
- {
- // Find difference between [desired to be Asserted] and those LEDs
- // that are physically asserted currently.
- std::set_difference(
- temp.begin(), temp.end(), currentState.begin(), currentState.end(),
- std::inserter(ledsAssert, ledsAssert.begin()), ledComp);
- }
-
- // Update the current actual and desired(the virtual actual)
- currentState = std::move(temp);
- combinedState = std::move(desiredState);
+ ledStateMap = newState;
// If we survive, then set the state accordingly.
return assert;
diff --git a/manager/manager.hpp b/manager/manager.hpp
index 4e611b8..24a6c17 100644
--- a/manager/manager.hpp
+++ b/manager/manager.hpp
@@ -10,6 +10,9 @@
#include <string>
#include <unordered_map>
+// to better see what the string is representing
+using LedName = std::string;
+
namespace phosphor
{
namespace led
@@ -89,6 +92,10 @@
// Nothing here
}
+ /* create the resulting map from all currently asserted groups */
+ static auto getNewMap(std::set<const ActionSet*> assertedGroups)
+ -> std::map<LedName, Layout::LedAction>;
+
/** @brief Given a group name, applies the action on the group
*
* @param[in] path - dbus path of group
@@ -146,13 +153,8 @@
/** @brief Pointers to groups that are in asserted state */
std::set<const ActionSet*> assertedGroups;
- /** @brief Contains the highest priority actions for all
- * asserted LEDs.
- */
- ActionSet currentState;
-
- /** @brief Contains the set of all actions for asserted LEDs */
- ActionSet combinedState;
+ /** Map of led name to current state */
+ std::map<std::string, Layout::LedAction> ledStateMap;
/** @brief Custom callback when enabled lamp test */
std::function<bool(ActionSet& ledsAssert, ActionSet& ledsDeAssert)>