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

Send AMD when AME is received #205

Merged
merged 4 commits into from
Sep 21, 2022
Merged

Conversation

bobjacobsen
Copy link
Contributor

No description provided.

@@ -88,6 +88,13 @@ public void processFrame(OpenLcbCanFrame f) {
return; // as a convenience, ignore
}

if (f.isAliasMapDefinition()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be testing for enquiry?
Also we need to verify that the payload of the enquiry is empty or matches our node ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done, I think.

Copy link
Collaborator

@balazsracz balazsracz left a comment

Choose a reason for hiding this comment

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

I think this code is now correct. I would be happy to approve.
However, there may be an issue somewhere.

  • The previous code was generating unnecessary but compliant messages. (We can always send an extra AMD.)
  • I received a report yesterday that JMRI 5.1.4 gets into an infinite packet loop if there are two JMRIs on the same network.
  • monitor log here: https://sites.google.com/site/balazsracz/jmri

I can not explain the infinite packet loop even in the presence of the previous code. Bob, do you have any idea?

public int getNIDa() {
return nida.getNIDa();
}

/**
* @return True of frame matches current NodeID
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo (of/if)

@bobjacobsen
Copy link
Contributor Author

I've never seen that AMD loop, but I only rarely have two JMRIs on the same network. Will see if I can recreate.

Looking at the trace, it seems that each JMRI is somehow using multiple aliases at the same time; I don't know how that's possible.

@balazsracz
Copy link
Collaborator

balazsracz commented Sep 21, 2022 via email

@bobjacobsen
Copy link
Contributor Author

Right. Tried that without immediate success. But there's some latency there, so if it's e.g. a problem with overlapping processing that might not provoke it.

Have also put two JMRI's on a single physical network (two RR-Cirkits boards and an LCC Clock) with separate adapters, no luck. Will continue having two up while working, in case it's a rare event.

@balazsracz
Copy link
Collaborator

balazsracz commented Sep 21, 2022 via email

@bobjacobsen
Copy link
Contributor Author

OK, seen it. It's due to the problem you found in review at the top: Sending AMD in response to AMD rather than AME. Fixed by this PR. I'll put a note in the JMRI 5.1.4 "new problems" section.

@balazsracz
Copy link
Collaborator

balazsracz commented Sep 21, 2022 via email

@bobjacobsen
Copy link
Contributor Author

Working on it. There's also something wrong with the current version of this PR, also working on that.

@bobjacobsen
Copy link
Contributor Author

I was reusing the "f" frame variable, which put a local-alias AMD in it, which the rest of the processing reacted to. That's why it was cycling the alias: It thought the frame was from an external device with the same alias it had.

There may still be an existing issue here with whether it's properly restarting the CIM/RIM sequence when this happens in certain states, based on reading the code, but I don't have time to make that a priority right now.

@balazsracz balazsracz merged commit 64a191e into openlcb:master Sep 21, 2022
@balazsracz
Copy link
Collaborator

I built a release with this fix, you can push it to JMRI from the usual place: https://repo.maven.apache.org/maven2/org/openlcb/openlcb/0.7.31/

@bobjacobsen
Copy link
Contributor Author

bobjacobsen commented Sep 21, 2022 via email

@bobjacobsen bobjacobsen deleted the send-AMD-to-AME branch March 1, 2023 15:53
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.

None yet

2 participants