Multiple issues with app_level.c

[JonasJ]

HI,

I've noticed a few issues with the example app_level.c.

1: The g_set_transition() was updated in the 3.0.0 release to avoid a division by zero, however, this fix introduced a bug. Notice that line 9 below uses params.move.required_move  instead of the correct params.set.required_delta. This is easy to fix, simply remove line 9 as the check is the same for both TRANSITION_SET and TRANSITION_DELTA_SET.

static bool g_set_transition(void * p_data)
{
    app_level_server_t * p_server = (app_level_server_t *) p_data;

    switch (p_server->state.transition_type)
    {
        /* Requirement: If transition time is not within valid range, do not start the transition. */
        case TRANSITION_SET:
            return (p_server->state.transition_time_ms > 0 && p_server->state.params.move.required_move != 0);
        case TRANSITION_DELTA_SET:
            return (p_server->state.transition_time_ms > 0 && p_server->state.params.set.required_delta != 0);

        /* Requirement: If transition time is not within valid range, or given move level (delta level)
        is zero, do not start the transition. */
        case TRANSITION_MOVE_SET:
            return (p_server->state.transition_time_ms > 0 &&
                    p_server->state.transition_time_ms != MODEL_TRANSITION_TIME_INVALID &&
                    p_server->state.params.move.required_move != 0);
        default:
            return false;
    }
}

While looking at this code, line 17 should either be removed or introduced for TRANSITION_(DELTA)_SET as well. The only time this can happen is if the user set a bad value in p_server->p_dtt_ms. The generic_level_server.c implementation ensures that the transition time is always valid when app_level.c receives it.

2: The transition "parsing" in generic_level_state_move_set_cb() contain some redundant code.

* Line 22 should be removed as the target_level is correctly set later.

* Line 28 to 35 should also be removed as the validation of the transition time has already been done by generic_level_server.c. This matches the behavior of the other two _set callbacks. The TRANSITION_TIME_STEP_100MS_MAX is also wrong, but that doesn't matter as it should be removed all together.

static void generic_level_state_move_set_cb(const generic_level_server_t * p_self,
                                            const access_message_rx_meta_t * p_meta,
                                            const generic_level_move_set_params_t * p_in,
                                            const model_transition_t * p_in_transition,
                                            generic_level_status_params_t * p_out)
{
    app_level_server_t   * p_server = PARENT_BY_FIELD_GET(app_level_server_t, server, p_self);

    /* Update internal representation of Level value, process timing. */
    if (p_in_transition == NULL)
    {
        p_server->state.delay_ms = 0;
        if (p_server->p_dtt_ms == NULL)
        {
            p_server->state.transition_time_ms = 0;
        }
        else
        {
            p_server->state.transition_time_ms = *p_server->p_dtt_ms;
        }

        p_server->state.target_level = p_server->state.present_level;
    }
    else
    {
        p_server->state.delay_ms = p_in_transition->delay_ms;

        /* Requirement: If transition time is out of range, transition cannot be started. However
        this must be stored to respond correctly to the get messages. */
        if (p_in_transition->transition_time_ms > TRANSITION_TIME_STEP_100MS_MAX)
        {
            p_server->state.transition_time_ms = MODEL_TRANSITION_TIME_INVALID;
        }
        else
        {
            p_server->state.transition_time_ms = p_in_transition->transition_time_ms;
        }
    }

3: Generic Move Set with a Delta Level (called move_level in the SDK for some reason) of 0 doesn't work properly. From the standard, "When a Generic Level Server receives the message with a value of the Delta Level field equal to 0, it shall stop changing the Generic Level state.". The current implementation doesn't handle this properly.

Since g_set_delay and g_set_transition both will return false, the only outcome in the transition state table is FSM_NO_ACTION. This is wrong and the correct action should be A_TRANSITION_COMPLETE.

    FSM_TRANSITION(E_MOVE_SET,        G_SET_DELAY,        A_DELAY_START,        S_IN_DELAY),
    FSM_TRANSITION(E_MOVE_SET,        G_SET_TRANSITION,   A_TRANSITION_START,   S_IN_TRANSITION),
    FSM_TRANSITION(E_MOVE_SET,        FSM_OTHERWISE,      FSM_NO_ACTION,        S_IDLE)

4: The function a_transition_complete() doesn't work properly for Generic Move Set. The assignment `p_server->state.present_level = p_server->state.target_level;` should only be done for all TRANSITION_MOVE_SET otherwise the present_level would be set to either INT16_MIN or INT16_MAX.

5: From section 7.4.1.3 "It is recommended than an additional status message is published within 1 second after a state transition starts if the transition time is 2 seconds or longer.". The implementation doesn't currently seem to follow this recommendation.

I'm using SDK For Mesh v3.1.0.

Thanks

[Mttrinh]

Hi,

Thank you for reporting this, I have reported this internally.

[JonasJ]

Another issue is with g_set_delay() that should ensure that p_server->state.params.move.required_move/p_server->state.params.set.required_delta are > 0.

Otherwise it will start the delay timer even if nothing should happen.

[JonasJ]

There are also issues with REMAINING_TIME and TRANSITION_COMPLETE macros. If the delay is longer than the transition time the Status sent in reply to a Get request will be wrong.

[PM Bubblynet]

Hi there! I wonder if there is any news? when the dimming will not be experimental anymore?

[Mttrinh]

Hi,

The issues in this post have been fixed in our latest Mesh SDK v3.2.0. Unfortunately, we do not have a timeline for when the dimming example is out of experimental.