Add c api to call user function for every successfully deframed SBP message. [ESD-998] #723

denniszollo · 2019-09-03T17:59:34Z

This allows a user to use sbp_process_generic and call the user function for every sbp message. It doesn't change our API at all and could simplify many things like logging SBP messages to a file without needing to copy over the sbp deframing code.

ESD-998

benjaminaltieri · 2019-09-03T18:24:00Z

had a backlog item to add this feature (https://swift-nav.atlassian.net/browse/ESD-998). thanks for picking this up!

benjaminaltieri

looks good. some other ideas we had were creating a SBP_MSG_RETURN_ALL define that you could assign callbacks with in order to achieve the same functionality within current framework but a question was how to forward the raw frames as opposed to the deframed data

benjaminaltieri · 2019-09-03T18:26:00Z

c/src/sbp.c

 */
 s8 sbp_process(sbp_state_t *s, s32 (*read)(u8 *buff, u32 n, void *context))
+{
+return sbp_process_generic(s, read, &sbp_process_payload);


benjaminaltieri · 2019-09-03T18:26:33Z

c/src/sbp.c

+}
+
+s8 sbp_process_generic(sbp_state_t *s, s32 (*read)(u8 *buff, u32 n, void *context),
+    s8 (*sbp_process_payload) (sbp_state_t *s, u16 sender_id, u16 msg_type, u8 msg_len, u8 payload[]))


probably want to avoid aliasing sbp_process_payload here with the arg name. also you might be able to reuse sbp_msg_callback_t if you pass the sbp_state_t as the context * arg

I renamed the existing sbp_process_payload to sbp_process_payload_cbs to notate that it goes through the linked list of callbacks. This one, I renamed to sbp_process_payload_fn to indicate it is a function pointer.

jbangelo · 2019-09-03T18:48:31Z

c/include/libsbp/sbp.h

 void sbp_state_init(sbp_state_t *s);
 void sbp_state_set_io_context(sbp_state_t *s, void* context);
 s8 sbp_process(sbp_state_t *s, s32 (*read)(u8 *buff, u32 n, void* context));
+s8 sbp_process_generic(sbp_state_t *s, s32 (*read)(u8 *buff, u32 n, void *context),


Nit: This is fairly generic name. Maybe something like sbp_process_msg_catchall or something like that? As an alternative I think a comment explaining what this is meant for would help. I know we don't have any comments for the other functions, but they are sorta-kinda self explanatory while this one isn't.

I renamed to sbp_process_all and added some comments per your suggestion.

jbangelo · 2019-09-03T18:50:58Z

c/include/libsbp/sbp.h

 void sbp_state_set_io_context(sbp_state_t *s, void* context);
 s8 sbp_process(sbp_state_t *s, s32 (*read)(u8 *buff, u32 n, void* context));
+s8 sbp_process_generic(sbp_state_t *s, s32 (*read)(u8 *buff, u32 n, void *context),
+    s8 (*sbp_process_payload) (sbp_state_t *s, u16 sender_id, u16 msg_type, u8 msg_len, u8 payload[]));


I'd be hesitant to have the generic callback take in sbp_state_t * directly, the user's code shouldn't be using that. Instead maybe have a void *context as an arg to the generic callback and pass it into sbp_process_generic as well?

Agreed, I think a context was better and more flexible. For the legacy case, the context is the head of the linked list.

jbangelo · 2019-09-03T18:51:43Z

c/src/sbp.c


        /* Message complete, process it. */
-        s8 ret = sbp_process_payload(s, s->sender_id, s->msg_type, s->msg_len,
+        s8 ret = (*sbp_process_payload)(s, s->sender_id, s->msg_type, s->msg_len,


If you give this arg a different name then you shouldn't need to deference the function pointer here.

jbangelo

LGTM! If/when we make a v2.0 of libsbp I think one of the alternative approaches we discussed would be preferable, but API stability is more important right now I think.

jbangelo · 2019-09-04T15:59:09Z

c/include/libsbp/sbp.h

+                   s8 (*sbp_process_payload) (u16 sender_id, u16 msg_type, u8 msg_len,
+                                             u8 payload[], void* context),
+                   void* process_payload_context);
+s8 sbp_process_payload_cbs(u16 sender_id, u16 msg_type, u8 msg_len,


I'm waffling on if sbp_process_payload_cbs() should be publicly declared in the header or not. On the one hand I don't think we would need to expose this to the consumer in most cases. But there is at least the case where the consumer might want to dynamically switch between using the registered callbacks and just getting everrything, in which case providing sbp_process_payload_cbs() is helpful...

I guess with how libsbp is currently structured it makes sense making this public. Ideally consumers wouldn't monkey around with the inner values of the libsbp structs, but they are already exposed and exposing this limits the amount of internal stuff they would need to deal with in the case I described.

silverjam

Couldn't this be reduced to one (or two) API additions and no API changes? That is

sbp_set_payload_cb(sbp_state_t* s, sbp_process_all_fn_t fn)

?

benjaminaltieri · 2019-09-04T17:21:40Z

c/include/libsbp/sbp.h

+ * deframed sbp message. This is useful for logging or forwarding any message
+ * regardless of type. Optionally, it can call sbp_process_payload_cbs itself
+ * to invoke matching callbacks in the linked list as well.*/
+s8 sbp_process_all(sbp_state_t *s, s32 (*read)(u8 *buff, u32 n, void *context),


I definitely prefer this naming scheme, but I agree with @silverjam that we shouldn't change the public API excepting for additions (just like we have to do with sbp messages)

if need be we can marshall sbp_process_payload directly to sbp_process_payload_cbs with a comment stating that this api is deprecated.

denniszollo · 2019-09-05T05:51:59Z

I've updated one more time in such a way that there is no change to existing API. I've also added unit tests.

benjaminaltieri

definitely closer to what was originally proposed for that backlog ticket. a couple ideas to integrate but looking good otherwise

benjaminaltieri · 2019-09-05T05:56:05Z

c/src/sbp.c

@@ -1,4 +1,4 @@
-/*
+ /*


missing asterisk and incorrect indentation

benjaminaltieri · 2019-09-05T05:59:05Z

c/src/sbp.c

+ * \return `SBP_OK` (0) if successful, `SBP_NULL_ERROR` if fn is NULL
+ *
+ */
+s8 sbp_register_allmsg_callback(sbp_state_t *s, sbp_allmsg_callback_fn_t fn,


I believe this is a 'set' operation, register implies multiple such callbacks can be registered which isn't true in this case

that is good point; I will rename to "sbp_set_allmsg_callback". Do you like the "allmsg" wording?

Now I am thinking to call it sbp_set_allmsg_fn() or something similar as to semantically differentiate between this feature, and the linked list of callbacks for particular message types.

agree I renamed

benjaminaltieri · 2019-09-05T05:59:26Z

c/src/sbp.c

+ * Register a callback that is called when any
+ * valid message is received.
+ *
+ * \param msg_type Message type associated with callback


this should be s SBP state context

benjaminaltieri · 2019-09-05T06:00:43Z

c/src/sbp.c

+s8 sbp_register_allmsg_callback(sbp_state_t *s, sbp_allmsg_callback_fn_t fn,
+                                void *context) {
+  /* Check our callback function pointer isn't NULL. */
+  if (0 == fn) {


NULL preferred for pointer checks

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.

benjaminaltieri · 2019-09-05T06:01:03Z

c/src/sbp.c

+ * \return `SBP_OK` (0) if successful
+ */
+s8 sbp_remove_allmsg_callback(sbp_state_t *s) {
+  s->allmsg_cb = 0;


NULL preferred

benjaminaltieri · 2019-09-05T06:02:58Z

c/src/sbp.c

  /* Reset the head of the callbacks list to NULL. */
  s->sbp_msg_callbacks_head = 0;
+  /* Reset the all_cb to 0. */
+  s->allmsg_cb = 0;


I would argue this shouldn't be included, sbp_remove_allmsg_callback already covers clearing this

I was debating that as well. If I don't use the word "callback" for the "allmsg_fuction", then this would be very clear and I wouldn't need to change sbp_clear_callbacks.

after renaming I agree and changed.

benjaminaltieri · 2019-09-05T06:03:53Z

c/src/sbp.c

      ret = SBP_OK_CALLBACK_EXECUTED;
    }
  }
+  if (0 != s->allmsg_cb) {


as discussed, separate PR to be up today

benjaminaltieri · 2019-09-05T06:04:46Z

c/src/sbp.c

    }
  }
+  if (0 != s->allmsg_cb) {
+    (*s->allmsg_cb)(sender_id, msg_type, msg_len, payload, s->allmsg_context);


Suggested change

(*s->allmsg_cb)(sender_id, msg_type, msg_len, payload, s->allmsg_context);

*(s->allmsg_cb)(sender_id, msg_type, msg_len, payload, s->allmsg_context);

or omit deref, it isn't technically necessary

silverjam · 2019-09-05T19:28:43Z

c/include/libsbp/version.h

 #define SBP_MINOR_VERSION 6
 /** Protocol patch version. */
-#define SBP_PATCH_VERSION 4
+#define SBP_PATCH_VERSION 6


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

silverjam · 2019-09-05T19:28:51Z

c/include/libsbp/sbp.h

 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);


How about sbp_register_allmsg_callback (or sbp_register_all_msg_callback) to match sbp_register_callback?

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

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.

silverjam · 2019-09-05T19:29:55Z

c/include/libsbp/sbp.h

  u8 msg_buff[256];
  void* io_context;
  sbp_msg_callbacks_node_t* sbp_msg_callbacks_head;
+  sbp_allmsg_fn_t allmsg_fn;


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

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

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

silverjam · 2019-09-05T19:31:26Z

c/src/sbp.c

+ * \return `SBP_OK` (0) if successful
+ */
+s8 sbp_remove_allmsg_fn(sbp_state_t *s) {
+  s->allmsg_fn = 0;


Clear allmsg_context too?

yes I agree.

silverjam

Looking good, a few minor nits from me though.

denniszollo · 2019-09-06T22:16:31Z

Superceded by #725

benjaminaltieri changed the title ~~Add c api to call user function for every successfully deframed SBP message.~~ Add c api to call user function for every successfully deframed SBP message. [ESD-998] Sep 3, 2019

benjaminaltieri reviewed Sep 3, 2019

View reviewed changes

jbangelo reviewed Sep 3, 2019

View reviewed changes

denniszollo force-pushed the sbp_process_so_callbacks_c branch 5 times, most recently from ec1f6b7 to 5f40085 Compare September 3, 2019 21:48

jbangelo approved these changes Sep 4, 2019

View reviewed changes

silverjam suggested changes Sep 4, 2019

View reviewed changes

benjaminaltieri reviewed Sep 4, 2019

View reviewed changes

denniszollo force-pushed the sbp_process_so_callbacks_c branch 5 times, most recently from 6df0bc6 to cba446b Compare September 5, 2019 05:49

benjaminaltieri reviewed Sep 5, 2019

View reviewed changes

denniszollo force-pushed the sbp_process_so_callbacks_c branch 2 times, most recently from 9252ddf to 097b116 Compare September 5, 2019 19:15

silverjam reviewed Sep 5, 2019

View reviewed changes

denniszollo force-pushed the sbp_process_so_callbacks_c branch from 097b116 to 03ecb92 Compare September 5, 2019 19:38

denniszollo added 3 commits September 5, 2019 12:45

Add allmsg callback API for libsbp C library

47474bd

Add allmsg API tests to c library

710ae26

update c version.h from latest github tag

7ca15c7

denniszollo force-pushed the sbp_process_so_callbacks_c branch from 03ecb92 to 7ca15c7 Compare September 5, 2019 19:47

denniszollo closed this Sep 6, 2019

	(*s->allmsg_cb)(sender_id, msg_type, msg_len, payload, s->allmsg_context);
	*(s->allmsg_cb)(sender_id, msg_type, msg_len, payload, s->allmsg_context);

Add c api to call user function for every successfully deframed SBP message. [ESD-998] #723

Add c api to call user function for every successfully deframed SBP message. [ESD-998] #723

Uh oh!

Conversation

denniszollo commented Sep 3, 2019 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjaminaltieri commented Sep 3, 2019

Uh oh!

benjaminaltieri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbangelo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverjam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminaltieri Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denniszollo commented Sep 5, 2019

Uh oh!

benjaminaltieri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denniszollo commented Sep 3, 2019 •

edited by atlassian bot

Loading

silverjam left a comment •

edited

Loading

benjaminaltieri Sep 4, 2019 •

edited

Loading

silverjam Sep 5, 2019 •

edited

Loading