Skip to content

Optionally truncate displayed arrays when _depth != 0 in text mode#60

Closed
amitchone wants to merge 4 commits into
mainfrom
text-encoder-truncate-arrays
Closed

Optionally truncate displayed arrays when _depth != 0 in text mode#60
amitchone wants to merge 4 commits into
mainfrom
text-encoder-truncate-arrays

Conversation

@amitchone
Copy link
Copy Markdown
Collaborator

As it says on the tin, example:

uart:~$ thingset ?HMCU
:85 {"testArray":[1,2,3,4,"..."],"rIpAddr":"192.168.1.1"}
uart:~$ thingset ?HMCU/testArray
:85 [1,2,3,4,5,6,7,8,9,10]

Comment thread zephyr/Kconfig
config THINGSET_PLUS_PLUS_TEXT_ARRAY_MAX
int "Max array elements to render in text mode (0 = unlimited)"
depends on THINGSET_PLUS_PLUS_PROTOCOL_TEXT
range 0 65535
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

May 16-bit systems never die.

@garethpotter
Copy link
Copy Markdown
Collaborator

:85 {"testArray":[1,2,3,4,"..."] }

To me, the ellipsis being in quotes (while syntactically correct JSON) looks very odd.

If this is only for display - and it must be, because it is useless as a transmission format - then it does not matter that it is not correct JSON.

@amitchone
Copy link
Copy Markdown
Collaborator Author

:85 {"testArray":[1,2,3,4,"..."] }

To me, the ellipsis being in quotes (while syntactically correct JSON) looks very odd.

If this is only for display - and it must be, because it is useless as a transmission format - then it does not matter that it is not correct JSON.

Yes... I was torn between being syntactially valid and the aesthetic... But you raise a good point, there is no genuine use case where it needs to be valid JSON so I will remove the quotes

@amitchone
Copy link
Copy Markdown
Collaborator Author

Feels a lot better, actually...

uart:~$ thingset ?HMCU
:85 {"testArray":[1,2,3,4,...],"rIpAddr":"192.168.1.1"}
uart:~$ thingset ?HMCU/testArray
:85 [1,2,3,4,5,6,7,8,9,10]
uart:~$ 

@garethpotter
Copy link
Copy Markdown
Collaborator

I would also remove the Oxford comma before the ellipsis.

@garethpotter
Copy link
Copy Markdown
Collaborator

But I can see an argument for not removing it.

@amitchone
Copy link
Copy Markdown
Collaborator Author

I have no preference either way to be honest... It's trivial to remove

@amitchone
Copy link
Copy Markdown
Collaborator Author

uart:~$ thingset ?HMCU
:85 {"testArray":[1,2,3,4...],"rIpAddr":"192.168.1.1"}

versus

uart:~$ thingset ?HMCU
:85 {"testArray":[1,2,3,4,...],"rIpAddr":"192.168.1.1"}

@amitchone
Copy link
Copy Markdown
Collaborator Author

@garethpotter the prevailing (read two additional) opinion is keep the comma

@garethpotter
Copy link
Copy Markdown
Collaborator

@garethpotter the prevailing (read two additional) opinion is keep the comma

To be expected from a bunch of people in Oxford really.

Copy link
Copy Markdown
Collaborator

@garethpotter garethpotter left a comment

Choose a reason for hiding this comment

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

As noted in my comments, I think this will have to be a runtime option, because a server cannot return truncated arrays to a network client.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

To actually engage with the substance of this, the slight issue I have with this is that it leaks what is a distinctly text-only concern, and a subset of a text-only concern at that (i.e. rendering for display) into the base class. Maybe this is fine, but I can't help wondering if there is another way.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Responding to myself, the argument in favour of the current implementation is that encodeListSeparator and encodeKeyValuePairSeparator are both already in the base class and are no-ops in binary mode.

Comment thread zephyr/Kconfig
bool "Enable text mode"
default true

config THINGSET_PLUS_PLUS_TEXT_ARRAY_MAX
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think this can be a compile-time option. The serial renderer might want to truncate but if a client connects over the network and queries the system in text mode (as is its right), it should expect to get the full data back.

Comment on lines +242 to +244
{
return size;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This method body should be in the .cpp file.

Comment on lines +249 to +251
{
return true;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As above.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just as we have ThingSetBinaryDecoderOptions, perhaps we want:

enum ThingSetTextEncoderOptions
{
    abbreviateArrays = 1 << 0
};

@amitchone
Copy link
Copy Markdown
Collaborator Author

Closed in favour of #62

@amitchone amitchone closed this Apr 30, 2026
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