-
Notifications
You must be signed in to change notification settings - Fork 9
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
Handle Simple bits on InitializationComplete and VerifiedNode messages #263
Handle Simple bits on InitializationComplete and VerifiedNode messages #263
Conversation
|
||
static final int MTI_VERIFY_NID = 0x10A4; | ||
static final int MTI_VERIFIED_NID = 0x10B0; | ||
static final int MTI_VERIFIED_NID_SIMPLE = 0x10B1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these values are wrong. I believe they are from pre-2012 when the MTI numbers were different.
I hope they are not used in the code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As near as I can tell these values and the associated getMTI()
method aren't used anywhere. I'll remove them at some point.
@ThreadSafe | ||
public class InitializationCompleteSimpleMessage extends InitializationCompleteMessage { | ||
|
||
public InitializationCompleteSimpleMessage(NodeID source) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we do it with a new class, we should add a function isSimpleProtocolSubset()
that is false in the base class and true here.
Alternatively, we could avoid making new classes and instead take the "isSimple" bit on the constructor. The producer/consumer identified messages do that; when one m,essage has variants that are different in some minor ways.
I don't have a strong preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next PR will do this with a property on the messages and hasSimpleProtocol()
@@ -239,6 +239,11 @@ List<Message> processFormat1(CanFrame f) { | |||
case InitializationComplete: | |||
retlist.add(new InitializationCompleteMessage(source)); | |||
return retlist; | |||
|
|||
case InitializationCompleteSimple: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add unit tests into https://github.com/openlcb/OpenLCB_Java/blob/master/test/org/openlcb/can/MessageBuilderTest.java
We need two tests per message type: one rendering messages into can frames and one p[arsing a can frame into the message.
Closed in favor of the solution in #264 |
See Issue #254 for background