Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to reset an encoder #165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Ability to reset an encoder #165

wants to merge 1 commit into from

Conversation

TSonono
Copy link

@TSonono TSonono commented Oct 9, 2019

Added a method that allows for resetting an encoder (Start writing from the beginning of its allocated buffer space.). This is very useful when you quickly want to discard everything that has currently been encoded by that encoder.

Also, a mixup in the documentation was adjusted (cborencoder.c).

…rom the beginning of the buffer). This is very useful when you quickly want to discard everything that has currently been encoded by that encoder. Also adjustet a mixup in the documentation (cborencoder.c)
@thiagomacieira
Copy link
Member

Thank you for the contribution. Let me see if I understand your use-case: you want to use the buffer like a circular buffer, so you can discard what has already been used and simply continue on, without discarding the state?

I think it's a valid use-case, but I have a couple of issues:

  1. I don't know if I want to store the pointer in the CborEncoder structure. Would it make sense to pass the pointer to cbor_encoder_reset instead?
  2. Maybe call it simply cbor_encoder_set_buffer and take both the new begin pointer and size? That way, you can relocate to a new buffer instead of overwriting.
  3. Need to check if there's any API that backtracks and tries to read a byte already written. I don't think they exist anymore, but we need to be sure.

@TSonono
Copy link
Author

TSonono commented Oct 9, 2019

The reason I put a start pointer in the CborEncoder struct was for a scenario where you are somewhere in the code where you don’t have necessarliy have access to a reference to the buffer you are encoding to.

I actually think both a cbor_encoder_reset and a cbor_encoder_set_buffer function could be useful and be used in different scenarios.

@thiagomacieira
Copy link
Member

If you're somewhere down in the code and don't have the original pointer, how do you know it's ok to reset? The only point in resetting would be after you've read the data out and sent it to the next layer (socket, file, encryption, etc.). For that, you need the begin pointer again.

@TSonono
Copy link
Author

TSonono commented Oct 10, 2019

In my use case, I want to reset before I’ve read it out. Essentially, I have something encoded that I have decided I want to discard, and start over with that encoded message.

Maybe I’m not clear or maybe it’s not good practice?

@thiagomacieira
Copy link
Member

Your change resets the buffer but does not reset the state. It doesn't make sense to discard the data previously written, since you need it o make sense of what will come next.

@TSonono
Copy link
Author

TSonono commented Oct 10, 2019

I see. If I then would have access to the buffer reference, in the case of cbor_encoder_set_buffer, would something like this be sufficient or do you also require some state handling?

void cbor_encoder_set_buffer(CborEncoder *enoder, uint8_t *buffer, size_t size)
{
encoder->data.ptr = buffer;
encoder->end = buffer + size;
}

@thiagomacieira
Copy link
Member

This function is fine. We just need to document that it does not reset the state. If you want a clean state, you can just use cbor_encoder_init again.

@@ -433,7 +434,7 @@ static CborError encode_string(CborEncoder *encoder, size_t length, uint8_t shif
*/

/**
* Appends the text string \a string of length \a length to the CBOR stream
* Appends the byte string \a string of length \a length to the CBOR stream
* provided by \a encoder. CBOR requires that \a string be valid UTF-8, but
* TinyCBOR makes no verification of correctness.
Copy link

Choose a reason for hiding this comment

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

Suggested change
* TinyCBOR makes no verification of correctness.

@@ -433,7 +434,7 @@ static CborError encode_string(CborEncoder *encoder, size_t length, uint8_t shif
*/

/**
* Appends the text string \a string of length \a length to the CBOR stream
* Appends the byte string \a string of length \a length to the CBOR stream
* provided by \a encoder. CBOR requires that \a string be valid UTF-8, but
Copy link

Choose a reason for hiding this comment

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

Suggested change
* provided by \a encoder. CBOR requires that \a string be valid UTF-8, but
* provided by \a encoder. CBOR byte strings are arbitrary raw data.

@@ -445,7 +446,7 @@ CborError cbor_encode_byte_string(CborEncoder *encoder, const uint8_t *string, s
}

/**
* Appends the byte string \a string of length \a length to the CBOR stream
* Appends the text string \a string of length \a length to the CBOR stream
* provided by \a encoder. CBOR byte strings are arbitrary raw data.
Copy link

Choose a reason for hiding this comment

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

Suggested change
* provided by \a encoder. CBOR byte strings are arbitrary raw data.
* provided by \a encoder. CBOR requires that \a string be valid UTF-8, but
* TinyCBOR makes no verification of correctness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants