Add nrx (Navtex) sentence #54
Conversation
|
Thanks for the PR. I'm currently quite busy, I will have a look as soon as time allows. |
…y responsibility to replace special characters in message part of NRX
…e not the same. Thus, more test cases added to cover those parts
|
Hello Mario, Were you able to set free time to take a look at my pull request? |
|
Next week I will be able to. |
There was a problem hiding this comment.
First things first, thank you for the PR. In general it looks nice, you kept consistence mostly to existing code.
Besides the findings in the review (code suggestions), which are mostly about consistency, there are some issues I'd like to address:
- In general, members are do not have a prefix
m_, but a suffix of_, please honor consistency. - Generally, the library tries to prevent misuse. However, the member function
nrx:::set_message()does not . It allows the user to set an arbitrary string to be set, which can cause the sentence to be more than maximum sentence characters long. This should be prevented. - The individual parsing tests should be done in one test, looping over a container of sentences.
- Looking around the internet, there seems to be that the same NRX sentence to be passed around, sometimes even with wrong checksums, caused by the hex number encodings. This seems to be an quite small sample.
- Since this is a multi-sentence message, similar to VDO/VDM messages, what are you're thoughts about handling the overall message? Similar to
examples/parse_ais.cpp? This would also be the best place to decode escapes within the message.
include/marnav/nmea/nrx.hpp
Outdated
| void set_total_characters(uint32_t t) noexcept { m_total_characters = t; } | ||
| void set_total_bad_characters(uint32_t t) noexcept { m_total_bad_characters = t; } | ||
| void set_status(status s) noexcept { m_status = s; } | ||
| void set_message(const std::string & m) noexcept { m_message = m; } |
There was a problem hiding this comment.
This looks incomplete.
The implementation should be in the cpp file. It should not be noexcept, because it should check for max string length.
Co-authored-by: Mario Konrad <[email protected]>
Co-authored-by: Mario Konrad <[email protected]>
Co-authored-by: Mario Konrad <[email protected]>
Co-authored-by: Mario Konrad <[email protected]>
Co-authored-by: Mario Konrad <[email protected]>
Co-authored-by: Mario Konrad <[email protected]>
Co-authored-by: Mario Konrad <[email protected]>
Co-authored-by: Mario Konrad <[email protected]>
|
Hello Mario, Thank you for your response. Most of the issues you realized are resolved and pushed. I'll fix the set_message() function by handling special characters. Also, I'll try to make an example usage for NRX like the one in examples/parse_ais.cpp as soon. Best, |
|
It's been completed I suppose. Would you mind checking it out. |
|
Will have a look this week. Thanks! |
|
Hello Mario, Were you able to set free time to take a look at my pull request? |
|
Hi there. |
NRX (Navtex) sentence added