COVESA / vsomeip

An implementation of Scalable service-Oriented MiddlewarE over IP
Mozilla Public License 2.0
1.11k stars 691 forks source link

Is there something wrong in debounce related code? #580

Open lvmy3 opened 10 months ago

lvmy3 commented 10 months ago

image

In the implementation/routing/event.cpp file, if a duplicate event were received but not field notification, the "must_forward" variable would be true or the "is_allowed" would be 0x01. However, the expected result is that the duplicate event message should not be forwarded and the "epsilon_changefunc" should be executed to judge whether the event changes. Right?

lvmy3 commented 10 months ago

Source code is here: `std::set event::get_filtered_subscribers(bool _force) {

std::set<client_t> its_subscribers(get_subscribers());
std::set<client_t> its_filtered_subscribers;

std::shared_ptr<payload> its_payload, its_payload_update;
{
    its_payload = current_->get_payload();
    its_payload_update = update_->get_payload();
}

if (filters_.empty()) {

    bool must_forward = (type_ != event_type_e::ET_FIELD
            || _force
            || epsilon_change_func_(its_payload, its_payload_update));

    if (must_forward)
        return its_subscribers;

} else {
    byte_t is_allowed(0xff);

    std::lock_guard<std::mutex> its_lock(filters_mutex_);
    for (const auto s : its_subscribers) {

        auto its_specific = filters_.find(s);
        if (its_specific != filters_.end()) {
            if (its_specific->second(its_payload, its_payload_update))
                its_filtered_subscribers.insert(s);
        } else {
            if (is_allowed == 0xff) {
                is_allowed = (type_ != event_type_e::ET_FIELD
                    || _force
                    || epsilon_change_func_(its_payload, its_payload_update)
                    ? 0x01 : 0x00);
            }

            if (is_allowed == 0x01)
                its_filtered_subscribers.insert(s);
        }
    }
}

return its_filtered_subscribers;

}`

lutzbichler commented 10 months ago

The idea about this is/was that an event has no state and therefore should not be debounced (as it is lost and there is no way to ask for the current state). We may change this in a future version as we also got the request to debounce events from another source.

lvmy3 commented 10 months ago

The idea about this is/was that an event has no state and therefore should not be debounced (as it is lost and there is no way to ask for the current state). We may change this in a future version as we also got the request to debounce events from another source.

Thank you for your response! So, you mean that the debounce part only works for field? But if an event is a field notification, in this code the epsilon_changefunc will never be executed (since type_ != event_type_e::ET_FIELD is true). In this case, what is the point to set this epsilon_changefunc? In addition, if a field doesn't change, the server won't send a notification to clients. So, I think maybe this part is useless. Am I wrong?

lutzbichler commented 10 months ago

"event" instances representing fields are of type "event_type::ETFIELD". Therefore "type != event_type_e::ET_FIELD" evaluates to "false" for them and (if not forced) the epsilon function is evaluated. In case of "real" events, it evaluates to true and therefore enables sending, even if the event value did not change.

lvmy3 commented 10 months ago

"event" instances representing fields are of type "event_type::ETFIELD". Therefore "type != event_type_e::ET_FIELD" evaluates to "false" for them and (if not forced) the epsilon function is evaluated. In case of "real" events, it evaluates to true and therefore enables sending, even if the event value did not change.

My mistake! But if a field doesn't change, the server won't send a notification to clients, right? Then, the epsilon function will always return true, right? Since when the clients received notification, the field must have changed.