Small bugs found in the Dimming server example

[Jeffe]

The first bug: I believe that the target level argument for the definition of the transition callback should be of type int16_t instead of the unsigned version since it is inconsistent with the get and set callbacks. Also iirc according to the generic level model the range is defined between the min and max of the signed int16_t instead of uint16_t.

This doesn't really affect the functionality of the level server (lights work perfectly fine), but it leads to the RTT server logs raising some eyebrows, such as when you set the target level below 0, the rtt of the server side overflows to 65,535 as what an unsigned integer would be expected to do.

This bug is quite trivial to fix, I just change the callback typedef to the signed version and fix the related locations (in this case the app_level_server_transition_cb() function definition and forward declaration)

The second bug: with the dimming example is that the client server seems to be 1 cycle off from outputting the present level of the server when it recieves the status message. To clarify what i mean, this is a picture of the RTT output:

A small note: the server RTT outputs you see here are after I fixed the first bug

Anyways, the top terminal is the server and the bottom terminal is the client. The client RTT output of the current level always seems to be late by one command. Unfortunately I have not had to time to solve this bug unlike the one I mentioned first in this opening post, but perhaps later on I will try to find a fix unless someone beats me to it :P. One suspicion I have is that the client is outputting the message into the RTT *before* it actually receives the appropriate status message hence why its output is always one command late.

[tesc]

Hi,

Thank you for the reports, it is highly appreciated!

The first issue might be a bug, yes. I have reported it through our internal issue tracker. I agree the inconsistency looks out of place. Please note, however, that while the callbacks for set and get are used also for deltas (meaning both positive and negative values might apply), the transition target_level is an absolute value. Are you sure the level itself is not actually supposed to be an unsigned value?

Regards,
Terje

[Jeffe]

I can't see how the level is ever supposed to be an unsigned value because the delta level, like the set level and move level, is also represented by a signed type (int32_t to be specific). The generic level model seems to define all the level values as a signed integer

To clarify what I find odd, here is a picture of the output (no fixes, just the original dimming examples)

The top rtt output is the client and the bottom rtt output is the server

You can see that the set command in the client side is the target level is the correct signed type but in the server output it is the unsigned version. For example, in the last input from the client where it sets the target level from -20000 to -30000, on the server side it outputs the target level as 35536. I think its clear that its a mistake due to the uint16_t argument from the transition callback casting a signed int16_t input as its argument to a uint16. In this case it turns -30000 to 35536, but those two numbers exact same representation in binary which is indicative of a signed -> unsigned casting.

[tesc]

Hi,

Yes, you are right, generic level should be a signed value. According to the Mesh Model specification v. 1.0.1, section 3.1.2 Generic Level: "The Generic Level state is a 16-bit signed integer (2's complement) representing the state of an element."

Definitely a bug. Thanks!

Regards,
Terje