Skip to content

Conversation

rjwats
Copy link
Owner

@rjwats rjwats commented May 27, 2020

No description provided.

@rjwats
Copy link
Owner Author

rjwats commented May 27, 2020

Thinking of removing JsonSerializer/JsonDeserializer and replacing them with the following signatures in StatefulService.h:

enum class StateUpdateResult { 
  CHANGED = 0, // The update changed the state and propagation should take place if required
  UNCHANGED, // The state was unchanged, propagation should not take place
  ERROR // There was a problem updating the state, propagation should not take place
};

template <class T>
using StateUpdater = StateUpdateResult (*)(T& settings);
template <class T>
using JsonStateUpdater = StateUpdateResult (*)(JsonObject& root, T& settings);

template <class T>
using StateReader = void (*)(const T& settings);
template <class T>
using JsonStateReader = void (*)(T& settings, JsonObject& root);

Comment on lines 38 to 45
static UpdateOutcome update(JsonObject& root, LightState& lightState) {
boolean newState = root["led_on"] | DEFAULT_LED_STATE;
if (lightState.ledOn != newState) {
lightState.ledOn = newState;
return UpdateOutcome::CHANGED;
}
return UpdateOutcome::UNCHANGED;
}
Copy link
Contributor

@kasedy kasedy May 27, 2020

Choose a reason for hiding this comment

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

How about to extend the example with a brightness param. It is a good use case to show how to make validation. Slider can go from 0 to 10 with 10% brightness change per snap. Update function will validate that is brightness in a given range and map it into analogWrite range from 0 to PWMRANGE (1023) with map function. I would also keep on-off switch.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Maybe let's address the larger change first.

Comment on lines 72 to 78
beginTransaction();
UpdateOutcome outcome = updateFunction(_state);
if (outcome == UpdateOutcome::CHANGED) {
callUpdateHandlers(originId);
}
endTransaction();
return outcome;
Copy link
Contributor

@kasedy kasedy May 27, 2020

Choose a reason for hiding this comment

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

I have an idea with auto-transaction. I'll prepare a PR into this branch to show my findings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll do it later as a separate PR.

@rjwats
Copy link
Owner Author

rjwats commented May 27, 2020

I will sort out updating the docs tomorrow.

@rjwats
Copy link
Owner Author

rjwats commented May 28, 2020

Docs updated, how are we looking?

README.md Outdated
Comment on lines 401 to 406
```cpp
lightStateService.update([&](LightState& state) {
state.on = true; // turn on the lights!
return StateUpdateResult::CHANGED; // notify StatefulService of the change
}, "timer");
```
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
```cpp
lightStateService.update([&](LightState& state) {
state.on = true; // turn on the lights!
return StateUpdateResult::CHANGED; // notify StatefulService of the change
}, "timer");
```
```cpp
lightStateService.update([&](LightState& state) {
if (state.on) {
return StateUpdateResult::UNCHANGED;
}
state.on = true; // turn on the lights!
return StateUpdateResult::CHANGED; // notify StatefulService of the change
}, "timer");

Comment on lines 92 to 98
beginTransaction();
StateUpdateResult result = stateUpdater(jsonObject, _state);
if (result == StateUpdateResult::CHANGED) {
callUpdateHandlers(originId);
}
endTransaction();
return result;
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
beginTransaction();
StateUpdateResult result = stateUpdater(jsonObject, _state);
if (result == StateUpdateResult::CHANGED) {
callUpdateHandlers(originId);
}
endTransaction();
return result;
beginTransaction();
StateUpdateResult result = stateUpdater(jsonObject, _state);
endTransaction();
if (result == StateUpdateResult::CHANGED) {
callUpdateHandlers(originId);
}
return result;

We can close transaction much earlier, right after state changed.

Comment on lines 74 to 82
StateUpdateResult update(std::function<StateUpdateResult(T&)> stateUpdater, const String& originId) {
beginTransaction();
StateUpdateResult result = stateUpdater(_state);
if (result == StateUpdateResult::CHANGED) {
callUpdateHandlers(originId);
}
endTransaction();
return result;
}
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
StateUpdateResult update(std::function<StateUpdateResult(T&)> stateUpdater, const String& originId) {
beginTransaction();
StateUpdateResult result = stateUpdater(_state);
if (result == StateUpdateResult::CHANGED) {
callUpdateHandlers(originId);
}
endTransaction();
return result;
}
StateUpdateResult update(std::function<StateUpdateResult(T&)> stateUpdater, const String& originId) {
beginTransaction();
StateUpdateResult result = stateUpdater(_state);
endTransaction();
if (result == StateUpdateResult::CHANGED) {
callUpdateHandlers(originId);
}
return result;
}

@rjwats
Copy link
Owner Author

rjwats commented May 29, 2020

Thanks for the changes @kasedy - I'll get these changes made and merged into master after work today.

My plan is to then update and re-open your access point fixes PR - also fixing the other bugs I've spotted.

Call update handlers outside of mutex
Fix invalid example in README.md
@kasedy
Copy link
Contributor

kasedy commented May 29, 2020

LGTM

@rjwats rjwats merged commit 0d39c5c into master May 29, 2020
@rjwats rjwats deleted the apply_updates2 branch May 29, 2020 19:58
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.

2 participants