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

parsing binary vs hex data #3

Open
thedinuka opened this issue Mar 24, 2013 · 2 comments
Open

parsing binary vs hex data #3

thedinuka opened this issue Mar 24, 2013 · 2 comments
Assignees
Labels

Comments

@thedinuka
Copy link
Contributor

When parsing m_nData in function fromJson() the lenght of cTmp is checked to figure out whether the data was sent as binary or hex. I'm a bit worried about hard coding the threshold value for this decision (ie number 12). Is there anything we can do about this?

@ghost ghost assigned mylifeofagony Mar 24, 2013
@mylifeofagony
Copy link

Hello Dinuka,

When parsing m_nData in function fromJson() the lenght of cTmp is checked

to figure out whether the data was sent as binary or hex. I'm a bit worried
about hard coding the threshold value for this decision (ie number 12). Is
there anything we can do about this?

binary was only added to make the code compatible with the old firmware
v1.2. As this is working properly now, we can drop all the stuff that won't
be needed anymore. So, binary representation should be removed completely.

I had a chat with Pete and James (who is going to handle the Node client

devs) Friday, and there we agreed to revise the protocol a bit, considering
a suggestion made by you some time back. Basically we decided not to use
JSON in the DA field as discussed previously. Instead, the encoding, pulse
width and payload will be concatenated to a single hex string and will be
put in the DA field.
So the DA field should look like
| one byte encoding | 1 reserved byte | 4 byte pulse width | variable
number of bytes data |

OK, sounds good.

In any case, we still need to keep track of the encoding and pulse width

for each 433 device. When decoding RF packets, this information can simply
be added to the m_nData without any modifications to the rest of the code.
So I can go ahead and implement that. When receiving data from the Node, I
think it's best to add two members to the Json packet (encoding and pulse
width), so that when the encoding is done in the onBoardManager::handle(),
I can chose the correct encoder to use based on those members. Let me know
what you think.

I had a look at how you implemented the header and was wondering why you
chose a ull for that? That's 8 bytes, and the header is only 4 bytes. But I
would suggest to have seperate fields for the different values in the class
anyway. Makes the code a lot more understandable. I changed and committed
the code. Let me know if you like it...

Cheers,
Mike

@mylifeofagony
Copy link

I just realized that I can't test the code right now, so I will commit it
later when I am home.

I will also add parsing functionality for the new header and remove the
binary stuff. That means that the code won't work with the old Node client
anymore...

Mike

On Mon, Mar 25, 2013 at 12:18 PM, Mike van Dyke [email protected]:

Hello Dinuka,

When parsing m_nData in function fromJson() the lenght of cTmp is checked

to figure out whether the data was sent as binary or hex. I'm a bit worried
about hard coding the threshold value for this decision (ie number 12). Is
there anything we can do about this?

binary was only added to make the code compatible with the old firmware
v1.2. As this is working properly now, we can drop all the stuff that won't
be needed anymore. So, binary representation should be removed completely.

I had a chat with Pete and James (who is going to handle the Node client

devs) Friday, and there we agreed to revise the protocol a bit, considering
a suggestion made by you some time back. Basically we decided not to use
JSON in the DA field as discussed previously. Instead, the encoding, pulse
width and payload will be concatenated to a single hex string and will be
put in the DA field.
So the DA field should look like
| one byte encoding | 1 reserved byte | 4 byte pulse width | variable
number of bytes data |

OK, sounds good.

In any case, we still need to keep track of the encoding and pulse width

for each 433 device. When decoding RF packets, this information can simply
be added to the m_nData without any modifications to the rest of the code.
So I can go ahead and implement that. When receiving data from the Node, I
think it's best to add two members to the Json packet (encoding and pulse
width), so that when the encoding is done in the onBoardManager::handle(),
I can chose the correct encoder to use based on those members. Let me know
what you think.

I had a look at how you implemented the header and was wondering why you
chose a ull for that? That's 8 bytes, and the header is only 4 bytes. But I
would suggest to have seperate fields for the different values in the class
anyway. Makes the code a lot more understandable. I changed and committed
the code. Let me know if you like it...

Cheers,
Mike

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

No branches or pull requests

2 participants