Frame callback implementation & tests #725

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

This PR adds a new type of callback to the c SBP Library to handle the entire SBP frame. It leaves the external API unchanged except for these additions.

This new frame callback feature will allow for easier raw logging in applications including Piksi Multi's standalone logger. It also allows adding a frame callback of type SBP_MSG_ALL to fire a callback for any msg_type. A convenient helper function is added to the API for registering a callback for every msg_type. Implementation details were discussed internally quite a bit and I think this represents the consensus.

I've added unit tests for the new functionality and some missing unit tests like testing the max_size of SBP messages.

benjaminaltieri

looking great so far, really like the registration updates. lots of idea to explore here, I'm mostly looking to reduce the doubling up I'm seeing between frame and message buffers during sbp_process and the double list traversal going on in sbp_process_payload/sbp_process_frame

benjaminaltieri · 2019-09-07T22:04:47Z

c/include/libsbp/sbp.h

+  u16 frame_len;
  u8 n_read;
  u8 msg_buff[256];
+  u8 frame_buff[256 + 8];


can we define SBP_MAX_FRAME_LEN with this value and use it? also, the above line could use SBP_MAX_PAYLOAD_LEN

c/include/libsbp/sbp.h

benjaminaltieri · 2019-09-07T22:14:03Z

c/include/libsbp/sbp.h

+ * This enum is stored on each sbp_msg_callback_node struct to identify how
+ * to cast the callback function pointer stored within.
+ */
+enum sbp_cb_type{


missing space

benjaminaltieri · 2019-09-07T22:54:19Z

c/src/sbp.c

    if (s->msg_len - s->n_read <= 0) {
      s->n_read = 0;
      s->state = GET_CRC;
+      for (int i = 0; i < s->msg_len; i++)


looking at the code now, it seems as though msg_buff and frame_buff are redundant. reading directly into msg_type, sender_id, etc. seems like an optimization used since we weren't trying to record the whole frame. since it's only 8 extra bytes and we could just as easily extract those values from frame_buff at the appropriate time, I would propose simply removing msg_buff altogether and returning the usual msg[] pointer as an offset from the beginning of frame_buff

benjaminaltieri · 2019-09-07T22:56:34Z

c/src/sbp.c

      s->state = GET_MSG;
+      s->frame_buff[5] = s->msg_len;
+      /* length of frame is msg_len + header_len + 2 for CRC */
+      s->frame_len = s->msg_len + SBP_HEADER_LEN + SBP_CRC_LEN;


frame_len should be incremented on each read (assuming implementation of previous comment) and validated against this formula. this will allow early exits to reflect the actual bytes held in frame_buff when we add frame error handlers

benjaminaltieri · 2019-09-07T23:01:27Z

c/src/sbp.c

+         * In future, it may be wise to add new return codes for the truth
+         * table of both process function return codes. For now, we return only
+         * sbp_process_payload's return code for backwards compatibility. */
+        sbp_process_frame(s, s->sender_id, s->msg_type, s->msg_len, s->msg_buff,


I would propose we allow sbp_process_frame to take an argument such that it calls all or a subset of callback types. this would allow sbp_process_payload to be a specialized call to sbp_process_frame where only SBP_TYPE_CALLBACK nodes are executed (similar to how you are extending the callback register functions

this would reduce code duplication between the two methods as well as avoid having to traverse the list twice.

benjaminaltieri · 2019-09-07T23:06:18Z

c/test/check_sbp.c

 void logging_reset(void)
 {
  n_callbacks_logged = 0;
+  n_frame_callbacks_logged = 0;


should the last_* variables get reset too?

yes, that would be better. updated.

benjaminaltieri · 2019-09-07T23:07:51Z

c/test/check_sbp.c

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


adding tests too! this is great, much good karma to you 🕉

❤️ ❤️ ❤️ ❤️ ❤️ ❤️

benjaminaltieri · 2019-09-07T23:13:38Z

c/include/libsbp/sbp.h

+ */
+enum sbp_cb_type{
+  SBP_TYPE_CALLBACK,
+  SBP_FRAME_CALLBACK


in true C style, we could turn this into a bit field to allow the sbp_process_frame function to be super flexible, just throw in a bitmask to select the callback styles you'd like to service. as easy as:

if ((node->cb_type & callback_select_mask) && ((node->msg_type == msg_type) || (node->msg_type == SBP_MSG_ALL))) {

benjaminaltieri · 2019-09-08T00:08:27Z

c/src/sbp.c

        s->n_read = 0;
+        /* set frame_buff to 0 after each preamble.
+         * Note, library functions are not used to avoid more dependencies. */
+        for (int i = 0; i < 255; i++) {


255 doesn't seem right here. since it's a static buffer we can use sizeof(s->frame_buff)

benjaminaltieri · 2019-09-08T04:46:52Z

Put these changes together to help illustrate the ideas: https://github.com/swift-nav/libsbp/tree/benaltieri/frame_callback_ideas

denniszollo · 2019-09-11T22:09:28Z

Ready for review.

silverjam

Mostly doc suggestions and pontificating about big vs little endian. Looking much better though.

c/src/sbp.c

silverjam · 2019-09-12T19:48:30Z

c/src/sbp.c

+      if (expected_frame_len != s->frame_len) {
+        return SBP_READ_TOO_MANY_ERROR;
+      }
      /* Swap bytes to little endian. */


TL;DR Leaving the rest of this here for posterity, but I think we should remove these comments, since it's not correct. To fully handle endianness issues we need to rewrite some of the generated code to handle it too, so not in the purview of this PR.

---8<---

Is this comment correct? The value is stored in little endian (if the machine it's generated on is little endian), we're not really swapping anything.

Using *((u16) &s->frame_buff[SBP_FRAME_OFFSET_CRC(s->msg_len)]) will work the same. E.g. https://godbolt.org/z/MZ1MCX -- Not saying this shouldn't be here, but we need to do the byte swap when storing the value on big endian for this to make sense.

That is, when we build an SBP packet, we should have something like:

#define BUF_TO_U16(buf, offset) *((u16*)&buf[0]) #ifdef __BIG_ENDIAN__ #define HTON16(crc) ((crc << 8) | crc) #else // __LITTLE_ENDIAN__ #define HTON16(crc) (crc) #endif

Then when generating the CRC for an outgoging SBP packet:

s->crc = HTON16(crc);

(Note our "network" byte order is little endian, which is opposite of the usual meaning of "network byte order").

--->8---

Per this good feedback I removed the bogus comment and created a helper function to cast to u8s to a u16. I would love to generate real parsing functions that are platform independent for each field for SBP with the generator and hope to accomplish that.

Big endian seems pretty uncommon lately, so I think it's a "nice to have", we have definitely seen (1 or 2) customers with this issue, but it shouldn't be high priority.

c/src/sbp.c

silverjam · 2019-09-12T20:37:14Z

c/test/check_sbp.c

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


❤️ ❤️ ❤️ ❤️ ❤️ ❤️

denniszollo · 2019-09-13T19:02:15Z

Doxygen looks pretty good:

denniszollo · 2019-09-13T19:50:59Z

c/include/libsbp/sbp.h

+ * to cast the callback function pointer stored within.
+ */
+enum sbp_cb_type {
+  SBP_TYPE_CALLBACK = 0,


I am starting to think this should be SBP_PAYLOAD_CALLBACK instead of SBP_TYPE_CALLBACK. I had named it type callback when it was impossible to register a frame callback on a particular message type.

denniszollo · 2019-09-13T20:08:56Z

I've made a few changes. I added a helper function to go from u8[] to u16 and remove confusing endianness comments, renamed SBP_TYPE_CALLBACK to SBP_PAYLOAD_CB, and updated the docs a lot per Jason's suggestions and thoughts. If approved I will plan to rebase and merge as is with the relatively fine-grained commit history.

If requested, I am willing and able to squash into three logical commits.

code changes for frame_callback
comment and doc updates for frame callback
unit test additions for frame_callback

silverjam · 2019-09-14T00:57:42Z

@denniszollo I think some squashing is in order, whatever seems appropriate to you is fine.

denniszollo · 2019-09-16T17:23:37Z

After working with this new API quite a bit on Friday, it became clear to me that the new return code (SBP_READ_TOO_MANY_ERROR) could result in a change in behavior depending on how the *read function pointer was implemented, as such Ben and I decided to remove it. I am going to merge the newly squashed version of this with only this one change.

benjaminaltieri · 2019-09-16T18:00:14Z

@denniszollo squashing helped, my only nit would be that the macro commit should be before the commit that uses the macros implemented there. one example is SBP_MAX_FRAME_LEN use in sbp_state_t my suggestion would be to squash that or just reorder. The reason being is that if someone is looking at the full code in that single functional commit, they will not see the definition of those macros

* classify callbacks into SBP_FRAME_CALLBACK(new) and SBP_PAYLOAD_CALLBACK (original) * add API for an ALL message cb to c lib * add macro for frame length calculation * c_frame_callback: add sbp_u8_array_to_u16 for sbp_process.

* c_frame_callback: Add unit tests for all_msg callback api * c_frame_callback: Add unit test for max length (255) to c lib

denniszollo mentioned this pull request Sep 6, 2019

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

Closed

denniszollo force-pushed the frame_callback branch 2 times, most recently from 1c91afd to 63eb051 Compare September 6, 2019 22:48

denniszollo requested a review from benjamin0 September 6, 2019 22:51

denniszollo force-pushed the frame_callback branch from eb99016 to 73ed3e4 Compare September 6, 2019 22:58

benjamin0 requested review from benjaminaltieri and removed request for benjamin0 September 6, 2019 23:35

benjaminaltieri reviewed Sep 7, 2019

View reviewed changes

benjaminaltieri reviewed Sep 8, 2019

View reviewed changes

anth-cole requested a review from benjaminaltieri September 12, 2019 16:47

benjaminaltieri approved these changes Sep 12, 2019

View reviewed changes

silverjam suggested changes Sep 12, 2019

View reviewed changes

denniszollo force-pushed the frame_callback branch from 467ff4a to 317a1ca Compare September 13, 2019 18:34

denniszollo commented Sep 13, 2019

View reviewed changes

silverjam approved these changes Sep 14, 2019

View reviewed changes

denniszollo force-pushed the frame_callback branch from 99dea33 to 0d13602 Compare September 16, 2019 17:22

denniszollo added 5 commits September 16, 2019 11:03

c_frame_callback: macros for frame callback (sbp.h)

3e238ca

c_frame_callback: whitespace changes to sbp.h

f1e0f28

c_frame_callback: add frame callback api to c lib

48ce9b3

* classify callbacks into SBP_FRAME_CALLBACK(new) and SBP_PAYLOAD_CALLBACK (original) * add API for an ALL message cb to c lib * add macro for frame length calculation * c_frame_callback: add sbp_u8_array_to_u16 for sbp_process.

c_frame_callback: update existing docs (sbp.c)

27ec7b8

c_frame_callback: add frame_callback unit tests

7b9ac44

* c_frame_callback: Add unit tests for all_msg callback api * c_frame_callback: Add unit test for max length (255) to c lib

denniszollo force-pushed the frame_callback branch from 0d13602 to 7b9ac44 Compare September 16, 2019 18:03

denniszollo merged commit 39c6625 into swift-nav:master Sep 16, 2019

Frame callback implementation & tests #725

Frame callback implementation & tests #725

Uh oh!

Conversation

denniszollo commented Sep 6, 2019

Uh oh!

benjaminaltieri 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!

Choose a reason for hiding this comment

Uh oh!

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!

benjaminaltieri commented Sep 8, 2019

Uh oh!

denniszollo commented Sep 11, 2019

Uh oh!

silverjam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denniszollo commented Sep 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denniszollo commented Sep 13, 2019

Uh oh!

silverjam commented Sep 14, 2019

Uh oh!

denniszollo commented Sep 16, 2019

Uh oh!

benjaminaltieri commented Sep 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benjaminaltieri left a comment •

edited

Loading