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

Bug with first slave with many PDOs and DC enabled. #243

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lucap85
Copy link

@lucap85 lucap85 commented Jan 11, 2019

If the first slave has DC enabled and enouh PDOs to need more than [EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM] bytes in the process image, the first segment's size will be zero instead of EC_FIRSTDCDATAGRAM and this cause the wrongly reading for the wkc at receiving time.

…[EC_MAXLRWDATA - EC_FIRSTDCDATAGRA] bytes in the process image, the first segmentsize will be zero instead and this cause the wrongly reading for the wkc from then incoming messages
@nakarlsson
Copy link
Contributor

nakarlsson commented Jan 16, 2019

I'd like to get some additional information to verify the fix
Could you add a slaveinfo dump with that 1 slave
Can capture a wireshark from simple_test with that 1 slave

@lucap85
Copy link
Author

lucap85 commented Feb 6, 2019

Hi,

here they are:

slaveinfo_and_simpletest_results.zip

@ArthurKetels
Copy link
Contributor

Unfortunately the wireshark dump does not show how the fix should work. Slave 1 has an input size smaller than [EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM] (1486 - 20). So everything works normal.

Also I think the fix is incorrect. When having a first slave with a very large PDO size then the correct solution would be to remove the LRW datagram when the segment size = 0 (or replace it with a NOP datagram).

@nakarlsson
Copy link
Contributor

This fix needs some re-work , please update and repost a proper fix as Arthur suggested

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

3 participants