sys/fmt: add hex/bytes sequence converters by aabadie · Pull Request #8343 · RIOT-OS/RIOT

aabadie · 2018-01-10T09:22:09Z

Contribution description

This PR adds 2 new formatting function to the fmt module:

fmt_bytes_hex: converts an array of bytes to an array of hex bytes
fmt_hex_bytes: converts an array of hex bytes to an array of bytes

Since I twice had the same comment in #8264 and #7505 about this (because I need these converters there), I think it's useful to provide them in a common place.

I also added simple unittests for the new functions.

Issues/PRs references

Related to #8264 and #7505

kaspar030 · 2018-01-10T09:33:22Z

sys/fmt/fmt.c

+size_t fmt_bytes_hex(char *out, const uint8_t *ptr, size_t n)
+{
+    size_t i = 0;
+    while (i < n) {


all functions in fmt just return the number of bytes that would have written, should out be NULL

while your code might be more readable to some, better keep the style of the rest of the file.

Please replace the while loop with:

if (out) { while (n--) { out += fmt_byte_hex(out, *ptr++); } }

(untested, but should be equivalent)

kaspar030 · 2018-01-10T09:40:42Z

Would you mind splitting this PR and move the hex to byte converter in another?
The reason is that bytes to hex is clear how to do it, but I will discuss the need of that 32b array. ;)

kaspar030 · 2018-01-10T11:20:36Z

See aabadie#5. It saves 16b on ARM. Not sure if it is much of an improvement. ;) What do you think?

aabadie · 2018-01-10T11:31:51Z

What do you think?

It looks good and smaller.

aabadie · 2018-01-10T13:04:42Z

@kaspar030, I integrated your PR and extended the unit tests (including converting hex >= A).

miri64 · 2018-01-10T13:14:20Z

tests/unittests/tests-fmt/tests-fmt.c

+    bytes = fmt_bytes_hex(out, val, 1);
+    out[bytes] = '\0';
+    TEST_ASSERT_EQUAL_INT(2, bytes);
+    TEST_ASSERT_EQUAL_STRING("AA", (char *) out);


Can you please at a n == 0 test also?

miri64 · 2018-01-10T13:15:25Z

tests/unittests/tests-fmt/tests-fmt.c

+    hex2[2] = '\0';
+    val1[0] = 0;
+    fmt_hex_bytes(val1, hex2);
+    TEST_ASSERT_EQUAL_INT(0xAB, val1[0]);


Here it would also be nice to test what happens if you hand over an empty string ;-).

And with strings "strlen(str) % 2 != 0" ;-)

miri64 · 2018-01-10T13:34:01Z

sys/include/fmt.h

+/**
+ * @brief Converts a sequence of hex bytes to an array of bytes
+ *
+ * The sequence of hex characters must have an odd length:


The tests (and your code) suggest that they have to have an even length. Also it is undocumented what happens if you provide a string of the wrong length.

The tests (and your code) suggest that they have to have an even length

Indeed, I always do this mistake...

miri64 · 2018-01-10T13:42:38Z

tests/unittests/tests-fmt/tests-fmt.c

+    TEST_ASSERT_EQUAL_INT(3, bytes);
+    TEST_ASSERT_EQUAL_INT(1, val3[0]);
+    TEST_ASSERT_EQUAL_INT(2, val3[1]);
+    TEST_ASSERT_EQUAL_INT(0xAF, val3[2]);


What happens if a character in the string is not in [0-9a-fA-F]?

it doesn't work. It's possible to update but I want to keep this simple and trust the user. I added a doxygen comment for this.

kaspar030 · 2018-01-10T13:55:28Z

sys/fmt/fmt.c

+    if (out) {
+        while (n--) {
+            out += fmt_byte_hex(out, *ptr++);
+            len += 2;


This way len is broken if there's no out.

This should fix it: https://gist.github.com/06d9b04ed210aed79d42bd4bba885298

Maybe add a unittest for out==NULL but n > 0.

kaspar030 · 2018-01-10T13:58:24Z

@aabadie lot's of work for seemlingy simple functions, eh? ;) Thanks for doing it!

aabadie · 2018-01-10T15:37:31Z

lot's of work for seemlingy simple functions, eh?

I knew that would be the case but that's the game ;)

miri64 · 2018-01-10T15:37:31Z

sys/include/fmt.h

+ *
+ * The sequence of hex characters must have an even length:
+ * 2 hex character => 1 byte. If the sequence of hex has an odd length, this
+ * function returns 0 and a NULL out.


Maybe add the returns 0 to the documentation of @return as well.

miri64 · 2018-01-10T15:39:40Z

sys/include/fmt.h

+ *
+ * The sequence of hex characters must have an even length:
+ * 2 hex character => 1 byte. If the sequence of hex has an odd length, this
+ * function returns 0 and a NULL out.


a NULL out.:

use @p out to reference a parameter.

out is a pointer, not a pointer to a pointer, so returning NULL is not possible.

miri64 · 2018-01-10T15:40:43Z

sys/include/fmt.h

+ * function returns 0 and a NULL out.
+ *
+ * The hex characters sequence must contain valid hexadecimal characters
+ * otherwise this function returns an invalid byte array.


What is "an invalid byte array" in this case? Maybe a better wording would be "[…] otherwise the result in out will be undefined."

astralien3000 · 2018-01-10T16:38:03Z

tests/unittests/tests-fmt/tests-fmt.c

+    TEST_ASSERT_EQUAL_INT(1, bytes);
+    TEST_ASSERT_EQUAL_INT(0xEF, val1[0]);
+
+    char hex4[6] = "0102aF";


More hex4[7] ? Or hex4[] ?

miri64 · 2018-01-10T16:50:52Z

sys/include/fmt.h

+ *
+ * @param[out] out  Pointer to converted bytes, or NULL
+ * @param[in]  hex  Pointer to input buffer
+ * @returns    strlen(hex) / 2 for a valid sequence of hex bytes


Personal preference, but I would prefer another specification of what a "valid sequence" is here (e.g. "strlen(hex) / 2 when length of @p hex was even"

miri64

ACK, please squash.

- fmt_bytes_hex: converts an array of bytes to an array of hex bytes - fmt_hex_bytes: convert an array of hex bytes to an array of bytes

aabadie · 2018-01-10T20:32:45Z

squashed and rebased

aabadie added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Jan 10, 2018

aabadie mentioned this pull request Jan 10, 2018

drivers/rn2xx3: basic initial implementation (without netdev) #7505

Merged

kaspar030 reviewed Jan 10, 2018

View reviewed changes

aabadie force-pushed the pr/fmt_hex_bytes branch 2 times, most recently from 54296bf to d4364ae Compare January 10, 2018 13:03

miri64 requested changes Jan 10, 2018

View reviewed changes

miri64 reviewed Jan 10, 2018

View reviewed changes

aabadie force-pushed the pr/fmt_hex_bytes branch from d4364ae to f0d4a8f Compare January 10, 2018 13:25

miri64 requested changes Jan 10, 2018

View reviewed changes

kaspar030 requested changes Jan 10, 2018

View reviewed changes

miri64 requested changes Jan 10, 2018

View reviewed changes

astralien3000 reviewed Jan 10, 2018

View reviewed changes

miri64 reviewed Jan 10, 2018

View reviewed changes

sys/fmt: add new byte to hex converters:

2d02bc0

- fmt_bytes_hex: converts an array of bytes to an array of hex bytes - fmt_hex_bytes: convert an array of hex bytes to an array of bytes

aabadie force-pushed the pr/fmt_hex_bytes branch from 578c6af to 2a8e34d Compare January 10, 2018 20:30

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 10, 2018

tests/unittests: add fmt hex/bytes converter tests

2b6e426

aabadie force-pushed the pr/fmt_hex_bytes branch from 2a8e34d to 2b6e426 Compare January 10, 2018 20:50

miri64 approved these changes Jan 10, 2018

View reviewed changes

kaspar030 approved these changes Jan 11, 2018

View reviewed changes

kaspar030 merged commit 7a81386 into RIOT-OS:master Jan 11, 2018

aabadie added this to the Release 2018.01 milestone Jan 18, 2018

aabadie deleted the pr/fmt_hex_bytes branch February 26, 2018 10:31

Teufelchen1 mentioned this pull request Dec 19, 2023

tests: Slightly increase coverage of fmt unittests #20198

Merged

Conversation

aabadie commented Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

kaspar030 Jan 10, 2018 • 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!

kaspar030 commented Jan 10, 2018

Uh oh!

kaspar030 commented Jan 10, 2018

Uh oh!

aabadie commented Jan 10, 2018

Uh oh!

aabadie commented Jan 10, 2018

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!

kaspar030 commented Jan 10, 2018

Uh oh!

aabadie commented Jan 10, 2018

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!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jan 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aabadie commented Jan 10, 2018 •

edited

Loading

kaspar030 Jan 10, 2018 •

edited

Loading