ThingSet / thingset-device-library

ThingSet library for resource-constrained devices written in C/C++
https://thingset.io/thingset-device-library/
Apache License 2.0
13 stars 6 forks source link

question about codes #48

Open CUgroup opened 1 year ago

CUgroup commented 1 year ago

Greetings, Dr.jaeger. This is a student at CU. At present, we've finished connecting TS w/ CAN. I changed two places of your code in order to fit TS codes in CCS to support C2000, can you please check are these changes necessary or not?

  1. In ‘src’ folder of 'TS library', for every files, i.e., TS.c, TSbin.c, TS_txt.c, there are code sentence as:

for (unsigned int i = 0; i < n; i++)

It has an error in CCS, because Version of C/C++ GCC support in CCS is not advanced. Correct it as:

unsigned int i; for (i = 0; i < n; i++)

Do you want to change your code as above? Because it can get a wider usage, e.g. it can support other C standards, and it is easy to change.

  1. In ‘src’ folder, in 'thingset.c', one structure 'ts_get_object_by_id' is defined as:

/// struct ts_data_object ts_get_object_by_id (struct ts_context ts, ts_object_id_t id) { unsigned int i = 0;

for (i = 0; i < ts->num_objects; i++) {
    if (ts->data_objects[i].id == id) {
        return &(ts->data_objects[i]);
    }
}

return NULL; } ///

Afterwards,

endpoint = ts_get_object_by_id(ts, id);

W/out this change, 'response' = 0, because return of function is always 0, endpoint=0.

Delete ‘return NULL’, correct it as:

/// struct ts_data_object ts_get_object_by_id(struct ts_context ts, ts_object_id_t id) { unsigned int i = 0;

for (i = 0; i < ts->num_objects; i++) {
    if (ts->data_objects[i].id == id) {
        return &(ts->data_objects[i]);
    }
} 

///

afterwards, endpoint can be correct and can get correct 'response' as e.g. in TS website.

martinjaeger commented 1 year ago

I would be generally open for changes required to switch to C90 compatibility if you submit them as a PR. Is the upfront variable declaration you mentioned the only issue you experienced?

I don't completely understand your 2nd point. We can't just remove the return statement at the end. The function must return a value under all conditions. If the data object is not found it shall return NULL.

CUgroup commented 1 year ago

Appreciate it for responding, Dr.jaeger.

  1. It is the only occurring error, and it's quite easy to change, in seconds.
  2. Agree function needs to return under all conditions. Yet, originally code is,
struct ts_data_object *ts_get_object_by_id (struct ts_context *ts, ts_object_id_t id)
{
unsigned int i = 0;

for (i = 0; i < ts->num_objects; i++) {
    if (ts->data_objects[i].id == id) {
        return &(ts->data_objects[i]);
    }
}
return NULL;
}

Afterwards,

endpoint = ts_get_object_by_id(ts, id);

so, if (ts->data_objects[i].id == id, return &(ts->data_objects[i]), it is correct. Yet, return NULL is outside 'if' sentence, and outside 'for' sentence. i.e. under any condition, it'll always run 'return NULL' and get 'return = 0', And we tested it, it's true. If wanting to add condition data object is not found, we think correct one needs to be

struct ts_data_object *ts_get_object_by_id (struct ts_context *ts, ts_object_id_t id)
{
unsigned int i = 0;

for (i = 0; i < ts->num_objects; i++) {
    if (ts->data_objects[i].id == id) {
        return &(ts->data_objects[i]);
    }
    else {
        return NULL;
    }
}
}

Add 'else' to represent condition data object is not found, and it needs to be inside of 'for' sentence, not outside.

martinjaeger commented 1 year ago

Hello @CUgroup. I have revisited the improvements introduced in C99 and there is much more to it than just declaring variables for for loops outside the loop.

Within this library we make use of the following features of C99:

So I'm sorry to say we are not going to be able to switch to C90 support. Minimum requirements will be a C99 capable compiler.

Regarding your point 2: Your suggestion won't work. The loop iterates over all elements and it should not return NULL if the first element does not match with the provided id.

CUgroup commented 1 year ago

Greetings, Dr. Jaeger, appreciate it for your suggestions. In CCS, one option can change C standard to C99. After changing standard, code can run.

martinjaeger commented 1 year ago

That's great to hear.

BTW: I've been working on a new version of this library which has a more clean code structure, is better tested and uses an existing CBOR library. Currently it only works with Zephyr, but it won't be too difficult to enable support for other environments. It requires C99 as well.

https://github.com/ThingSet/thingset-node-c

Also the ThingSet specification has been updated to v0.6. The thingset-device-library supports v0.5 only, whereas the new thingset-node-c supports v0.6. See here for a changelog in the spec: https://thingset.io/spec/changelog.html#v0-5-to-v0-6