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

Observer Enhancement #7658

Closed
vwheeler63 opened this issue Jan 24, 2025 · 4 comments · Fixed by #7678
Closed

Observer Enhancement #7658

vwheeler63 opened this issue Jan 24, 2025 · 4 comments · Fixed by #7678

Comments

@vwheeler63
Copy link
Contributor

vwheeler63 commented Jan 24, 2025

Introduce the problem

While I was re-writing and reviewing the documentation for Observer, I noticed a couple of logic gaps, which I am predicting will be needed in the future:

  • Currently FLAG and STATE combinations can be set or cleared by an Observer based on EQUALITY or INEQUALITY with a reference value. However, I am predicting this will be also needed for the following integer comparison operations with <, <=, >=, > suggesting additional lv_obj_bind_...() functions. Note that only the first 2 of those could be implemented plus a "not" flag to control the execution path during testing, so it would only take 3 bits to manage (2 more bits in the flag_and_cond_t struct) to manage, no matter now it was organized. (No additional RAM cost.)

  • Each Subject carries a prev_value field, but the only place it is actually used is in these query functions:

    • lv_subject_get_previous_int()
    • lv_subject_get_previous_string()
    • lv_subject_get_previous_pointer()
    • lv_subject_get_previous_color()

While the latter is fine as a "minimal" implementation, there is an important UI design pattern that minimizes CPU overhead (extremely important in my opinion in some RTOS environments) that:

  • Only does an update on the Widgets ONCE immediately before each screen refresh, and

  • ONLY does so (for each Widget) when it will result in a visible change on the display, and this pattern especially avoids doing an update when the value displayed has not changed, since it incurs an lv_obj_invalidate(), which forces re-drawing, which is then a large-and-unnecessary source of CPU overhead that would result in no visible change on the display.

This pattern begs for Observer-notification filtering such that (when desired) that only sends out a notification when the NEW (incoming) value (in the various lv_subject_set_...() and lv_subject_copy_string() functions) is actually different from the previous value. All the logic (and data) to implement this capability this is already contained in the implementation except for the flag indicating when this filtering is desired.

Proposal

That the above features be added. I would definitely use it in our Dashboards if it were available to set a "widget_needs_updating" flag for each Widget so affected. It would simplify a lot of logic I have currently implemented to accomplish this (embedded in each screen's Update() function -- I have a screen "class" that always has an Update() function that gets called once, immediately before the display refresh).

I was thinking these features could perhaps be planned for the next major release (low priority).

Thoughts?

@kisvegabor
Copy link
Member

I agree with both.

  • <, <=, >=, >: I think having all of them is cleaner than 2 of them and NOT.
  • filtering: I think we should add the new_value != current_value test unconditionally, without having a flag to enable/disable it. The assumption is that the observers always know the current value and are in sync with it. Therefore setting the same value again shouldn't make any changes.

@vwheeler63
Copy link
Contributor Author

vwheeler63 commented Jan 28, 2025

  • I think having all of them is cleaner than 2 of them and NOT.

I concur.

  • I think we should add the new_value != current_value test unconditionally, without having a flag to enable/disable it. The assumption is that the observers always know the current value and are in sync with it. Therefore setting the same value again shouldn't make any changes.

Agreed! And I cannot think of a use case (especially with regard to Widget binding) where one would want a notification every time a value got written to (including when it didn't change). Interestingly, this popped out at me when updating the Observer documentation. For the documentation to match the code, I had to write "... notification ... when ... value is updated" vs "when value is changed" (which seemed odd).

@kisvegabor
Copy link
Member

For the documentation to match the code, I had to write "... notification ... when ... value is updated" vs "when value is changed" (which seemed odd).

It was a great catch indeed.

I'd like to emphasis that no notification should be triggered when when setting the same value because of the philosophy of subject-observers: the observers are always in sync with the subject because when they subscribe a notification will be fired to get the current value (probably it should be documented too) and when the value changes a notification is fired as well. So there is no way that notifying with the same value makes any difference, unless the user messes up something, but it's out of scope now. 😄

@vwheeler63
Copy link
Contributor Author

vwheeler63 commented Jan 29, 2025

probably it should be documented too

Done. Will be submitted in a subsequent PR. I also included that the notification goes out ONLY when the value has changed (involving changes in about 4 places in the document). I will submit this with the code update.

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 a pull request may close this issue.

2 participants