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
