chore: eliminate [in/out] parameters #94

zfields · 2023-09-08T03:45:55Z

No description provided.

m-mcgowan · 2023-09-08T07:46:55Z

n_helpers.c

        NOTE_C_LOG_ERROR("NULL parameter");
        return ERRSTR("NULL parameter", c_err);
    }

-    if (*outLen < cobsGuaranteedFit(inLen)) {
+    if (outLen < cobsGuaranteedFit(inLen)) {


cobsGuaranteedFit adds the +1 for the transport framing. I think we can make things more consistent, by making a clear separation between the pure COBS encoding requirements and the transport framing requirements (the +1.). Adding #define COBS_TRANSPORT_OVERHEAD=1 will make this easier to spot in the code.

I like this idea. To follow the common naming convention, I will use the symbol COBS_EOP_OVERHEAD.

From Wikipedia:

[OHB] : Overhead byte (Start of frame) 3+ -------------->| : Points to relative location of first zero symbol 2+-------->| : Is a zero data byte, pointing to next zero symbol [EOP] : Location of end-of-packet zero symbol. 0 1 2 3 4 5 : Byte Position 03 11 22 02 33 00 : COBS Data Frame 11 22 00 33 : Extracted Data OHB = Overhead Byte (Points to next zero symbol) EOP = End Of Packet

m-mcgowan · 2023-09-08T07:52:37Z

n_helpers.c

@@ -331,19 +340,63 @@ uint32_t NoteBinaryMaxEncodedLength(uint32_t unencodedLength)
    return cobsEncodedMaxLength(unencodedLength);


Needs a +1 for the framing overhead.

We can avoid littering the code with +1 everywhere, with

cobsEncodedToBufferLength(uint32_t encodedLength) { return encodedLength+COBS_TRANSPORT_OVERHEAD; }

That DRYs it up and means just a single change should the transport framing change, although here the DRY and clear semantics is more pertinent atm.

The code would use this whenever it needs to convert from the cobs encoding data length (such as the cobs value from the Notecard, or the max encoded length) to the actual buffer size needed to receive it.

now promoted up through cobsEncodedMaxLength()

m-mcgowan · 2023-09-08T07:54:08Z

n_helpers.c

-    if (!decodedLen) {
-        NOTE_C_LOG_ERROR("decodedLen cannot be NULL");
-        return ERRSTR("decodedLen cannot be NULL", c_err);
+    if (bufLen < (cobsEncodedMaxLength(decodedLen) + 1)) {


I think that when we want to know about the required size of the buffer we should use NoteBinaryMaxEncodedLength() since that's what we expect the callers to use.

Currently NoteBinaryMaxEncodedLength doesn't do the +1 so this call fails at present.

I thought we'd agreed on this last week that it's ok to call NoteBinaryMaxEncodedLength rather than replicating the code, especially when it's about validating buffer sizes.

(This is the 3rd time this has bit us btw.)

The cobsEncodedMaxLength() etc functions are suitable when talking about purely cobs conversion, and shouldn't make adjustments for the framing overhead.

I don't understand how the code you're referencing (line 419) failed; it incorporates the +1.

Assuming what is currently sitting on line 419 works, the ugly syntax will be resolved by the underlying change to cobsEncodedMaxLength().

NoteBinaryMaxEncodedLength didn't add the +1, so it was returning 1 byte less. I feel adding +1 to cobsXXX functions is mixing concerns.

I understand your concern and I don't disagree, but I don't have the freedom to modify cobsGuaranteedFit() (as it is derived from the Notecard firmware).

cobsEncodedMaxLength() was designed to be an analog to cobsGuaranteedFit(), and I believe it needs to follow precedent.

haydenroche5 · 2023-09-08T22:30:28Z

n_helpers.c

-  @param  len [out] the length of the decoded contents of the Notecard's binary
-          data store.
+  @param  encData The encoded binary data to decode.
+  @param  encLen The length of the encoded binary data.


Should we change encLen to encDataLen, to match how we use decData and decDataLen for NoteBinaryCodecEncode?

haydenroche5 · 2023-09-08T22:30:50Z

n_helpers.c

-                 length of the decoded data.
+  @param  decData The decoded binary data to encode.
+  @param  decDataLen The length of the decoded binary data.
+  @param  encBuf The target buffer for the encoded data. This can be in the same


This can be in the same - > This can be the same

I deliberately chose to add "in". The addresses MUST be different, but they can both be part of (or "in") the same buffer. I'm open to suggestions for rewording, but that point must be expressed.

I reworded it to be more clear and explicit regarding the concern expressed above.

haydenroche5 · 2023-09-08T22:32:31Z

n_helpers.c

+  @param  decDataLen The length of the decoded binary data.
+  @param  encBuf The target buffer for the encoded data. This can be in the same
+                 buffer as `decData`, allowing for in-place encoding (see note).
+  @param  encBufSize The size of `encBuf`.

  @returns  NULL on success, else an error string pointer.


This is outdated.

haydenroche5 · 2023-09-08T22:38:19Z

n_helpers.c

+          stored on the Notecard.
+
+  @param  len [out] The length required to hold the entire contents of the
+           Notecard's binary data store. If there's no data stored on the


270-271 are indented by 1 additional space. This is really unacceptable, @zfields.

haydenroche5 · 2023-09-08T22:39:23Z

n_helpers.c

 //**************************************************************************/
 /*!
  @brief  Receive a large binary object from the Notecard's binary buffer

-  @param  buffer A buffer to hold the binary object
-  @param  bufLen The total length of the provided buffer
+  @param  buffer        A buffer to hold the binary range


I'm good with either convention, but just noting that we aren't aligning the param descriptions for some of the other APIs in this PR. Also, prefer consistency with ending descriptions with a period.

n_helpers.c

haydenroche5 · 2023-09-08T22:43:30Z

n_helpers.c

    }

    // Return `NULL` if success, else error string pointer
    return NULL;
 }

+//**************************************************************************/
+/*!
+  @brief  Reset the Notecard's binary buffer.


Should we call it the binary data store / binary store instead of binary buffer? Same question applies elsewhere where we use the term binary buffer.

haydenroche5 · 2023-09-08T22:44:30Z

n_helpers.c

+        return err;
+    }
+
+    return NULL;


Need to free rsp. Should be caught by the unit test with valgrind; not sure why it wasn't on the CI run.

good catch!

haydenroche5 · 2023-09-08T22:48:14Z

n_helpers.c

@@ -564,19 +596,21 @@ const char * NoteBinaryTransmit(uint8_t *unencodedData, uint32_t unencodedLen,

            // Ensure the transaction doesn't return an error.
            if (!NoteRequest(req)) {
-                NOTE_C_LOG_ERROR("failed to initialize binary transaction");
+                const char *err = ERRSTR("failed to initialize binary transaction", c_err);
+                NOTE_C_LOG_ERROR(err);
                _UnlockNote();
                // On errors, we restore the caller's input buffer by COBS


Might make sense to remove "COBS" here, while you're making changes.

zfields added 2 commits September 8, 2023 03:40

chore: eliminate [in/out] parameters

a64be28

fix: astyle

d48d585

zfields requested review from m-mcgowan and haydenroche5 September 8, 2023 03:45

zfields added 3 commits September 8, 2023 04:14

test: improve coverage

97ee798

chore: rename binary RX API

cc3071a

fix: astyle

46a7b94

m-mcgowan suggested changes Sep 8, 2023

View reviewed changes

zfields added 17 commits September 8, 2023 13:09

chore: align cobsEncodedMaxLength with cobsGuaranteedFit

766fa8c

chore: Remove NoteBinaryReceiveAll()

1f450a7

chore: refactor NoteBinaryCodecDecode()

db4176e

chore: refactor NoteBinaryCodecEncode()

3a65491

chore: refactor NoteBinaryCodecMaxDecodedLength()

0941431

chore: refactor NoteBinaryCodecMaxEncodedLength()

1cf2669

chore: refactor NoteBinaryStoreDecodedLength()

91d90cc

chore: refactor NoteBinaryStoreEncodedLength()

70867b9

chore: refactor NoteBinaryStoreReset()

62a7943

chore: refactor NoteBinaryStoreReceive()

5e7d033

chore: refactor NoteBinaryStoreTransmit()

8538dde

fix: astyle

007ea17

fix: memory leak

a2d9884

reorder

fa5be7f

code formatting

f71e62e

fix: astyle

993c1f8

chore: enhance buffer check

7df7deb

haydenroche5 requested changes Sep 8, 2023

View reviewed changes

zfields force-pushed the zak-in-out branch from 16c72bc to 048c192 Compare September 8, 2023 23:42

chore: address PR feedback

ddd9d51

zfields force-pushed the zak-in-out branch from 048c192 to ddd9d51 Compare September 9, 2023 00:14

fix: Fix memory leak in NoteBinaryStoreReset.

209b099

haydenroche5 approved these changes Sep 9, 2023

View reviewed changes

zfields merged commit ff18fe8 into master Sep 9, 2023
4 checks passed

zfields deleted the zak-in-out branch September 9, 2023 01:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: eliminate [in/out] parameters #94

chore: eliminate [in/out] parameters #94

zfields commented Sep 8, 2023

m-mcgowan Sep 8, 2023

zfields Sep 8, 2023

m-mcgowan Sep 8, 2023

zfields Sep 8, 2023

m-mcgowan Sep 8, 2023

zfields Sep 8, 2023 •

edited

Loading

m-mcgowan Sep 8, 2023

zfields Sep 8, 2023 •

edited

Loading

haydenroche5 Sep 8, 2023

haydenroche5 Sep 8, 2023

zfields Sep 8, 2023 •

edited

Loading

zfields Sep 9, 2023

haydenroche5 Sep 8, 2023

haydenroche5 Sep 8, 2023

zfields Sep 8, 2023

haydenroche5 Sep 8, 2023

haydenroche5 Sep 8, 2023

haydenroche5 Sep 8, 2023 •

edited

Loading

zfields Sep 8, 2023

haydenroche5 Sep 8, 2023

		@@ -331,19 +340,63 @@ uint32_t NoteBinaryMaxEncodedLength(uint32_t unencodedLength)
		return cobsEncodedMaxLength(unencodedLength);

chore: eliminate [in/out] parameters #94

chore: eliminate [in/out] parameters #94

Conversation

zfields commented Sep 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfields Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfields Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfields Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydenroche5 Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfields Sep 8, 2023 •

edited

Loading

zfields Sep 8, 2023 •

edited

Loading

zfields Sep 8, 2023 •

edited

Loading

haydenroche5 Sep 8, 2023 •

edited

Loading