Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions c/include/libsbp/sbp.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ extern "C" {
#define SBP_OK_CALLBACK_EXECUTED 1
/** Return value indicating message decoded with no associated callback in sbp_process. */
#define SBP_OK_CALLBACK_UNDEFINED 2
/** Return value indicating message decoded and only an all_msg function was
* executed. */
#define SBP_OK_ALLMSG_FN_EXECUTED_ONLY 3
/** Return value indicating an error with the callback (function defined). */
#define SBP_CALLBACK_ERROR -1
/** Return value indicating a CRC error. */
Expand All @@ -46,6 +49,8 @@ extern "C" {

/** SBP callback function prototype definition. */
typedef void (*sbp_msg_callback_t)(u16 sender_id, u8 len, u8 msg[], void *context);
typedef void (*sbp_allmsg_fn_t)(u16 sender_id, u16 msg_type, u8 len, u8 msg[],
void *context);

/** SBP callback node.
* Forms a linked list of callbacks.
Expand Down Expand Up @@ -76,12 +81,20 @@ typedef struct {
u8 msg_buff[256];
void* io_context;
sbp_msg_callbacks_node_t* sbp_msg_callbacks_head;
sbp_allmsg_fn_t allmsg_fn;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No a huge fall of allmsg, would prefer all_msg, but I can live with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just didn't want anyone to be confused that all_msg meant all messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree on this point, considering one of the use cases of this function (hence the different signature) is to receive the entire raw frame so the binary blob can be written to file. from this perspective I would almost prefer a name like sbp_raw_frame_handler or something to that effect

void *allmsg_context;
} sbp_state_t;

/** \} */

/* registers a callback to be called on a particular msg_type */
s8 sbp_register_callback(sbp_state_t* s, u16 msg_type, sbp_msg_callback_t cb, void* context,
sbp_msg_callbacks_node_t *node);
/* sets a function to be called on ANY deframed message */
s8 sbp_set_allmsg_fn(sbp_state_t *s, sbp_allmsg_fn_t fn, void *context);
Copy link
Contributor

@silverjam silverjam Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about sbp_register_allmsg_callback (or sbp_register_all_msg_callback) to match sbp_register_callback?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I suggested set since this is not registered, as in place in a registry of callbacks along with other callbacks. there is only one allmsg function to either set or clear

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was worried about calling it a "callback". If I do, then I think it makes sense that sbp_clear_callbacks clears this allmsg callback as well, which is kind of a change in behavior for that part of the API that Ben wasn't too fond of.

/* gets the function to be called on ANY deframed message */
sbp_allmsg_fn_t sbp_get_allmsg_fn(sbp_state_t *s);
s8 sbp_remove_allmsg_fn(sbp_state_t *s);
s8 sbp_remove_callback(sbp_state_t *s, sbp_msg_callbacks_node_t *node);
void sbp_clear_callbacks(sbp_state_t* s);
void sbp_state_init(sbp_state_t *s);
Expand Down
2 changes: 1 addition & 1 deletion c/include/libsbp/version.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
/** Protocol minor version. */
#define SBP_MINOR_VERSION 6
/** Protocol patch version. */
#define SBP_PATCH_VERSION 4
#define SBP_PATCH_VERSION 6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should need to touch this, it's bumped automatically during a release.


/** \} */

Expand Down
67 changes: 66 additions & 1 deletion c/src/sbp.c
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,21 @@
*
* where `SBP_MY_MSG_TYPE` is the numerical identifier of your message type.
*
* If you'd like to call a function for EVERY sbp message, setup an allmsg function
* with the type sbp_allmsg_fn_t. It must be of the form:
*
* ~~~
* void my_allmsg_fn(u16 sender_id, u16 msg_type, u8 len, u8 msg[], void *context)
* {
* // Process msg.
* }
* ~~~
*
* Then register the allmsg callback with the SBP library as follows:
* ~~~
* sbp_register_allmsg_fn(&sbp_state, &my_allmsg_fn, &context);
* ~~~
*
* You must now call sbp_process() periodically whenever you have received SBP
* data to be processed, e.g. from the serial port. Remember sbp_process() may
* not use all available data so keep calling sbp_process() until all the
Expand Down Expand Up @@ -227,6 +242,44 @@ s8 sbp_remove_callback(sbp_state_t *s, sbp_msg_callbacks_node_t *node)
return SBP_CALLBACK_ERROR;
}

/** Set a function to be called for ANY message type.
*
* \param s pointer to sbp_state_t struct in use
* \param fn Pointer to message callback function
* \param context Pointer to context for callback function
* \return `SBP_OK` (0) if successful, `SBP_NULL_ERROR` if fn is NULL
*
*/
s8 sbp_set_allmsg_fn(sbp_state_t *s, sbp_allmsg_fn_t fn, void *context) {
/* Check our callback function pointer isn't NULL. */
if (0 == fn) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL preferred for pointer checks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but in order to do this it requires adding a new #include that wasn't here when the original author wrote. I thought perhaps there was a portability reason to not include stddefs. I'm happy to follow up with a separate PR to add in NULL for null pointer and the new include, but for now I wanted to follow the existing pattern.

return SBP_NULL_ERROR;
}
s->allmsg_fn = fn;
s->allmsg_context = context;
return SBP_OK;
}

/** Retrieve the callback for ANY message type.
*
* \param s pointer to sbp_state_t struct in use
* \return sbp_allmsg_fn_t* to allmsg fn pointer or NULL if none
*
*/
sbp_allmsg_fn_t sbp_get_allmsg_fn(sbp_state_t *s) {
return s->allmsg_fn;
}

/** Remove allmsg function pointer and context.
*
* \return `SBP_OK` (0) if successful
*/
s8 sbp_remove_allmsg_fn(sbp_state_t *s) {
s->allmsg_fn = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear allmsg_context too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I agree.

s->allmsg_context = 0;
return SBP_OK;
}

/** Clear all registered callbacks.
* This is probably only useful for testing but who knows!
*/
Expand All @@ -252,6 +305,9 @@ void sbp_state_init(sbp_state_t *s)

/* Clear the callbacks, if any, currently in s */
sbp_clear_callbacks(s);

/* Clear the allmsg_fn, if any, currently set on s */
sbp_remove_allmsg_fn(s);
}


Expand Down Expand Up @@ -302,7 +358,8 @@ void sbp_state_set_io_context(sbp_state_t *s, void *context)
* \return `SBP_OK` (0) if successful but no complete message yet,
* `SBP_OK_CALLBACK_EXECUTED` (1) if message decoded and callback executed,
* `SBP_OK_CALLBACK_UNDEFINED` (2) if message decoded with no associated
* callback, and `SBP_CRC_ERROR` (-2) if a CRC error
* callback, SBP_OK_ALLMSG_FN_EXECUTED_ONLY (3) if only the generic
* callback for every msg was called, and `SBP_CRC_ERROR` (-2) if a CRC error
* has occurred. Thus can check for >0 to ensure good processing.
*/
s8 sbp_process(sbp_state_t *s, s32 (*read)(u8 *buff, u32 n, void *context))
Expand Down Expand Up @@ -412,6 +469,8 @@ s8 sbp_process(sbp_state_t *s, s32 (*read)(u8 *buff, u32 n, void *context))
* \return `SBP_OK_CALLBACK_EXECUTED` (1) if message decoded and callback executed,
* `SBP_OK_CALLBACK_UNDEFINED` (2) if message decoded with no associated
* callback.
* `SBP_OK_ALLMSG_FN_EXECUTED_ONLY` (3) if message decoded and only
* allmsg callback was called
*/
s8 sbp_process_payload(sbp_state_t *s, u16 sender_id, u16 msg_type, u8 msg_len,
u8 payload[]) {
Expand All @@ -423,6 +482,12 @@ s8 sbp_process_payload(sbp_state_t *s, u16 sender_id, u16 msg_type, u8 msg_len,
ret = SBP_OK_CALLBACK_EXECUTED;
}
}
if (0 != s->allmsg_fn) {
(s->allmsg_fn)(sender_id, msg_type, msg_len, payload, s->allmsg_context);
if (ret == SBP_OK_CALLBACK_UNDEFINED) {
ret = SBP_OK_ALLMSG_FN_EXECUTED_ONLY;
}
}
return ret;
}

Expand Down
148 changes: 148 additions & 0 deletions c/test/check_sbp.c
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ void printy_callback(u16 sender_id, u8 len, u8 msg[], void* context)

u32 n_callbacks_logged;
u16 last_sender_id;
u16 last_msg_type;
u8 last_len;
u8 last_msg[256];
void* last_context;
Expand All @@ -101,6 +102,18 @@ void logging_callback(u16 sender_id, u8 len, u8 msg[], void* context)
/*printy_callback(sender_id, len, msg);*/
}

void logging_allmsg_fn(u16 sender_id, u16 msg_type, u8 len, u8 msg[], void* context)
{
n_callbacks_logged++;
last_sender_id = sender_id;
last_msg_type = msg_type;
last_len = len;
last_context = context;
memcpy(last_msg, msg, len);

/*printy_callback(sender_id, len, msg);*/
}

void test_callback(u16 sender_id, u8 len, u8 msg[], void* context)
{
/* Do nothing. */
Expand All @@ -118,6 +131,17 @@ void test_callback2(u16 sender_id, u8 len, u8 msg[], void* context)
(void)context;
}

void test_allmsg_fn(u16 sender_id, u16 msg_type, u8 len, u8 msg[],
void* context)
{
/* Do nothing. */
(void)sender_id;
(void)msg_type;
(void)len;
(void)msg;
(void)context;
}

sbp_msg_callbacks_node_t* sbp_find_callback(sbp_state_t *s, u16 msg_type)
{
/* If our list is empty, return NULL. */
Expand Down Expand Up @@ -451,6 +475,128 @@ START_TEST(test_callbacks)
}
END_TEST

START_TEST(test_allmsg_corner_cases)
{

sbp_state_t s;
sbp_state_init(&s);

/* Start with no callbacks registered. */
sbp_remove_allmsg_fn(&s);

fail_unless(sbp_set_allmsg_fn(&s, 0, 0) == SBP_NULL_ERROR,
"sbp_register_callback should return an error if cb is NULL");

fail_unless(sbp_set_allmsg_fn(&s, &test_allmsg_fn, 0) == SBP_OK,
"sbp_set_allmsg_fn should work");

fail_unless(sbp_get_allmsg_fn(&s) == &test_allmsg_fn,
"sbp_get_allmsg_fn should work");

sbp_remove_allmsg_fn(&s);

fail_unless(sbp_get_allmsg_fn(&s) == 0,
"sbp_get_allmsg_fn should now fail");
}
END_TEST

START_TEST(test_sbp_set_allmsg_cb)
{
/* TODO: Tests with different read function behaviour. */

sbp_state_t s;
sbp_state_init(&s);
sbp_state_set_io_context(&s, &DUMMY_MEMORY_FOR_IO);
logging_reset();
dummy_reset();
u8 ret = sbp_set_allmsg_fn(&s, &logging_allmsg_fn, &DUMMY_MEMORY_FOR_CALLBACKS);
fail_unless(ret == SBP_OK,
"registering all callback didn't work");

u8 test_data[] = { 0x01, 0x02, 0x03, 0x04 };

sbp_send_message(&s, 0x2269, 0x42, sizeof(test_data), test_data, &dummy_write);

while (dummy_rd < dummy_wr) {
fail_unless(sbp_process(&s, &dummy_read) >= SBP_OK,
"sbp_process threw an error!");
}

fail_unless(n_callbacks_logged == 1,
"one callback should have been logged");
fail_unless(last_sender_id == 0x42,
"sender_id decoded incorrectly");
fail_unless(last_len == sizeof(test_data),
"len decoded incorrectly. len is %u", last_len);
fail_unless(last_msg_type == 0x2269,
"msg_id decoded incorrectly");
fail_unless(memcmp(last_msg, test_data, sizeof(test_data))
== 0,
"test data decoded incorrectly");
fail_unless(last_context == &DUMMY_MEMORY_FOR_CALLBACKS,
"context pointer incorrectly passed");

/* now add a regular callback and make sure BOTH were called for the same message*/
static sbp_msg_callbacks_node_t n;
logging_reset();
dummy_reset();
sbp_register_callback(&s, 0x2269, &logging_callback, &DUMMY_MEMORY_FOR_CALLBACKS, &n);

sbp_send_message(&s, 0x2269, 0x42, sizeof(test_data), test_data, &dummy_write);

while (dummy_rd < dummy_wr) {
fail_unless(sbp_process(&s, &dummy_read) >= SBP_OK,
"sbp_process threw an error!");
}

/* should have two callbacks, one for all, one for normal */
fail_unless(n_callbacks_logged == 2,
"two callback should have been logged");
fail_unless(last_sender_id == 0x42,
"sender_id decoded incorrectly");
fail_unless(last_len == sizeof(test_data),
"len decoded incorrectly. len is %u", last_len);
fail_unless(last_msg_type == 0x2269,
"msg_id decoded incorrectly");
fail_unless(memcmp(last_msg, test_data, sizeof(test_data))
== 0,
"test data decoded incorrectly");
fail_unless(last_context == &DUMMY_MEMORY_FOR_CALLBACKS,
"context pointer incorrectly passed");

/* now remove the callback for all message types and make sure regular callback works */
logging_reset();
dummy_reset();
ret = sbp_remove_allmsg_fn(&s);
fail_unless(ret == SBP_OK,
"removing the all callback didn't work");
sbp_send_message(&s, 0x2269, 0x42, sizeof(test_data), test_data, &dummy_write);

while (dummy_rd < dummy_wr) {
fail_unless(sbp_process(&s, &dummy_read) >= SBP_OK,
"sbp_process threw an error!");
}
fail_unless(n_callbacks_logged == 1,
"one callbacks should have been logged");

/* now remove every callback */
logging_reset();
dummy_reset();
ret = sbp_remove_allmsg_fn(&s);
fail_unless(ret == SBP_OK,
"removing the all callback didn't work");
sbp_send_message(&s, 0x2269, 0x42, sizeof(test_data), test_data, &dummy_write);

while (dummy_rd < dummy_wr) {
fail_unless(sbp_process(&s, &dummy_read) >= SBP_OK,
"sbp_process threw an error!");
}
fail_unless(n_callbacks_logged == 1,
"one callbacks should have been logged");

}
END_TEST

Suite* sbp_suite(void)
{
Suite *s = suite_create("SBP");
Expand All @@ -461,6 +607,8 @@ Suite* sbp_suite(void)
tcase_add_test(tc_core, test_callbacks);
tcase_add_test(tc_core, test_sbp_send_message);
tcase_add_test(tc_core, test_sbp_process);
tcase_add_test(tc_core, test_allmsg_corner_cases);
tcase_add_test(tc_core, test_sbp_set_allmsg_cb);

suite_add_tcase(s, tc_core);

Expand Down