Skip to content

fix single device inputs/outputs overruning segment size #907

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.

Sign up for GitHub

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

Merged
nakarlsson merged 5 commits into from
May 5, 2025
Merged

fix single device inputs/outputs overruning segment size #907

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0b87f56
fix single device inputs/outputs overruning segment size
ddi-kkugler Apr 11, 2025
6a84071
update wkc calc
ddi-kkugler Apr 24, 2025
4bacdb0
refactor to pack segments tightly. allow segments 2-64 to be EC_MAXLR…
ddi-kkugler Apr 28, 2025
a440d6f
only split slave data between segments when it is larger than one seg…
ddi-kkugler Apr 29, 2025
f487702
only update wkc when we include data for the current device
ddi-kkugler Apr 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 44 additions & 29 deletions soem/ethercatconfig.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,7 +893,7 @@ static void ecx_config_find_mappings(ecx_contextt *context, uint8 group)
}
}

static void ecx_config_create_input_mappings(ecx_contextt *context, void *pIOmap,
static void ecx_config_create_input_mappings(ecx_contextt *context, void *pIOmap,
uint8 group, int16 slave, uint32 * LogAddr, uint8 * BitPos)
{
int BitCount = 0;
Expand Down Expand Up @@ -1012,8 +1012,8 @@ static void ecx_config_create_input_mappings(ecx_contextt *context, void *pIOmap
if (group)
{
context->slavelist[slave].inputs =
(uint8 *)(pIOmap) +
etohl(context->slavelist[slave].FMMU[FMMUc].LogStart) -
(uint8 *)(pIOmap) +
etohl(context->slavelist[slave].FMMU[FMMUc].LogStart) -
context->grouplist[group].logstartaddr;
}
else
Expand All @@ -1032,12 +1032,12 @@ static void ecx_config_create_input_mappings(ecx_contextt *context, void *pIOmap
}
context->slavelist[slave].FMMUunused = FMMUc;

/* Add one WKC for an input if flag is true */
/* Add one WKC for each input segment if flag is true */
if (AddToInputsWKC)
context->grouplist[group].inputsWKC++;
context->grouplist[group].inputsWKC += 1 + (FMMUsize / (EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM));
}

static void ecx_config_create_output_mappings(ecx_contextt *context, void *pIOmap,
static void ecx_config_create_output_mappings(ecx_contextt *context, void *pIOmap,
uint8 group, int16 slave, uint32 * LogAddr, uint8 * BitPos)
{
int BitCount = 0;
Expand Down Expand Up @@ -1150,14 +1150,14 @@ static void ecx_config_create_output_mappings(ecx_contextt *context, void *pIOma
if (group)
{
context->slavelist[slave].outputs =
(uint8 *)(pIOmap) +
etohl(context->slavelist[slave].FMMU[FMMUc].LogStart) -
(uint8 *)(pIOmap) +
etohl(context->slavelist[slave].FMMU[FMMUc].LogStart) -
context->grouplist[group].logstartaddr;
}
else
{
context->slavelist[slave].outputs =
(uint8 *)(pIOmap) +
(uint8 *)(pIOmap) +
etohl(context->slavelist[slave].FMMU[FMMUc].LogStart);
}
context->slavelist[slave].Ostartbit =
Expand All @@ -1170,9 +1170,9 @@ static void ecx_config_create_output_mappings(ecx_contextt *context, void *pIOma
FMMUc++;
}
context->slavelist[slave].FMMUunused = FMMUc;
/* Add one WKC for an output if flag is true */
/* Add one WKC for each output segment if flag is true */
if (AddToOutputsWKC)
context->grouplist[group].outputsWKC++;
context->grouplist[group].outputsWKC += 1 + (FMMUsize / (EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM));
Comment thread
ddi-kkugler marked this conversation as resolved.
Outdated
}

static int ecx_main_config_map_group(ecx_contextt *context, void *pIOmap, uint8 group, boolean forceByteAlignment)
Expand Down Expand Up @@ -1218,7 +1218,7 @@ static int ecx_main_config_map_group(ecx_contextt *context, void *pIOmap, uint8
LogAddr++;
BitPos = 0;
}
}
}

diff = LogAddr - oLogAddr;
oLogAddr = LogAddr;
Expand All @@ -1228,8 +1228,13 @@ static int ecx_main_config_map_group(ecx_contextt *context, void *pIOmap, uint8
if (currentsegment < (EC_MAXIOSEGMENTS - 1))
{
currentsegment++;
segmentsize = diff;
}
while (diff > (EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM) && currentsegment < (EC_MAXIOSEGMENTS - 1))
Copy link
Copy Markdown
Contributor

@nakarlsson nakarlsson Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if there are a clever way to split a "big" segment? I did a sample slave with 4B Out/1540B In.
The logic will create 3 frames when 2 frames easily is enough, but sometimes the clever logic comes with a cost in complexity to maintain and test.

When running overlapped the DC frame is sent alone in first segment.
Also, EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM , EC_FIRSTDCDATAGRAM is only relevant for first frame.

Any idea?

Copy link
Copy Markdown
Contributor Author

@ddi-kkugler ddi-kkugler Apr 28, 2025

Choose a reason for hiding this comment

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

Also, EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM , EC_FIRSTDCDATAGRAM is only relevant for first frame.

The code before was/is taking EC_FIRSTDCDATAGRAM into account on all segments, I can refactor and change that if you're confident it doesn't need this.

I wonder if there are a clever way to split a "big" segment? I did a sample slave with 4B Out/1540B In.
The logic will create 3 frames when 2 frames easily is enough, but sometimes the clever logic comes with a cost in complexity to maintain and test.

The code right now will pack a segment with as many slaves as will fit, it should be easy to extend that for this use case.

Copy link
Copy Markdown
Contributor

@nakarlsson nakarlsson Apr 29, 2025

Choose a reason for hiding this comment

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

Also, EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM , EC_FIRSTDCDATAGRAM is only relevant for first frame.

The code before was/is taking EC_FIRSTDCDATAGRAM into account on all segments, I can refactor and change that if you're confident it doesn't need this.

I wonder if there are a clever way to split a "big" segment? I did a sample slave with 4B Out/1540B In.
The logic will create 3 frames when 2 frames easily is enough, but sometimes the clever logic comes with a cost in complexity to maintain and test.

The code right now will pack a segment with as many slaves as will fit, it should be easy to extend that for this use case.

We can keep EC_FIRSTDCDATAGRAM to keep it simple, how would you otherwise be able to calculate an adjustment of WKC correctly? You don't know what segment it will belong to.

The logic should avoid splitting slave data between segments when possible.

Copy link
Copy Markdown
Contributor Author

@ddi-kkugler ddi-kkugler Apr 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The code is refactored so the wkc is cleared an incremented all within the config mapping functions rather than spread across separate functions as before. It will also pack large slave data into the current segment to potentially avoid using additional segments.

The logic should avoid splitting slave data between segments when possible.

I set it up this way and I'll agree this is just a preference for what "feels right". For what it's worth, the code can be shorter (and network traffic optimized) by always packing as tightly as possible and I can't find any spec that says a slave must be handled in a single PDU. The downside being that wireshark dumps might be harder to decipher with more optimized packing.

a cost in complexity to maintain and test

You mentioned maintenance and testing yesterday, how do you test this code? Is there a way to use SOES and run some sort of automated testing?

Copy link
Copy Markdown
Contributor

@nakarlsson nakarlsson Apr 30, 2025

Choose a reason for hiding this comment

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

The code is refactored so the wkc is cleared an incremented all within the config mapping functions rather than spread across separate functions as before. It will also pack large slave data into the current segment to potentially avoid using additional segments.

The logic should avoid splitting slave data between segments when possible.

I set it up this way and I'll agree this is just a preference for what "feels right". For what it's worth, the code can be shorter (and network traffic optimized) by always packing as tightly as possible and I can't find any spec that says a slave must be handled in a single PDU. The downside being that wireshark dumps might be harder to decipher with more optimized packing.

The rationale is to make Wireshark dumps as easy to dissect as possible.

a cost in complexity to maintain and test

You mentioned maintenance and testing yesterday, how do you test this code? Is there a way to use SOES and run some sort of automated testing?

I've created two SOES slaves on XMC48 that is 4B Out/1540B In and 1540B Out/4B In. I run manual tests.

Copy link
Copy Markdown
Contributor Author

@ddi-kkugler ddi-kkugler Apr 30, 2025

Choose a reason for hiding this comment

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

Thank you for the info, let me know if there is anything else you want me to do.

Copy link
Copy Markdown
Contributor

@nakarlsson nakarlsson Apr 30, 2025

Choose a reason for hiding this comment

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

It looks good! Nice work.
I'd like to do some more regression testing on bigger networks but I'll be out of office until next Monday, thank you for understanding it'll take a few more days before merge.

{
context->grouplist[group].IOsegment[currentsegment++] = (EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM);
diff -= (EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM);
}
segmentsize = diff;
}
else
{
Expand Down Expand Up @@ -1265,7 +1270,7 @@ static int ecx_main_config_map_group(ecx_contextt *context, void *pIOmap, uint8
if (!group)
{
context->slavelist[0].outputs = pIOmap;
context->slavelist[0].Obytes = LogAddr -
context->slavelist[0].Obytes = LogAddr -
context->grouplist[group].logstartaddr; /* store output bytes in master record */
}

Expand All @@ -1278,9 +1283,9 @@ static int ecx_main_config_map_group(ecx_contextt *context, void *pIOmap, uint8
/* create input mapping */
if (context->slavelist[slave].Ibits)
{

ecx_config_create_input_mappings(context, pIOmap, group, slave, &LogAddr, &BitPos);

if (forceByteAlignment)
{
/* Force byte alignment if the input is < 8 bits */
Expand All @@ -1289,7 +1294,7 @@ static int ecx_main_config_map_group(ecx_contextt *context, void *pIOmap, uint8
LogAddr++;
BitPos = 0;
}
}
}

diff = LogAddr - oLogAddr;
oLogAddr = LogAddr;
Expand All @@ -1299,8 +1304,13 @@ static int ecx_main_config_map_group(ecx_contextt *context, void *pIOmap, uint8
if (currentsegment < (EC_MAXIOSEGMENTS - 1))
{
currentsegment++;
segmentsize = diff;
}
while (diff > (EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM) && currentsegment < (EC_MAXIOSEGMENTS - 1))
{
context->grouplist[group].IOsegment[currentsegment++] = (EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM);
diff -= (EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM);
}
segmentsize = diff;
}
else
{
Expand Down Expand Up @@ -1348,14 +1358,14 @@ static int ecx_main_config_map_group(ecx_contextt *context, void *pIOmap, uint8
context->grouplist[group].IOsegment[currentsegment] = segmentsize;
context->grouplist[group].nsegments = currentsegment + 1;
context->grouplist[group].inputs = (uint8 *)(pIOmap) + context->grouplist[group].Obytes;
context->grouplist[group].Ibytes = LogAddr -
context->grouplist[group].logstartaddr -
context->grouplist[group].Ibytes = LogAddr -
context->grouplist[group].logstartaddr -
context->grouplist[group].Obytes;
if (!group)
{
context->slavelist[0].inputs = (uint8 *)(pIOmap) + context->slavelist[0].Obytes;
context->slavelist[0].Ibytes = LogAddr -
context->grouplist[group].logstartaddr -
context->slavelist[0].Ibytes = LogAddr -
context->grouplist[group].logstartaddr -
context->slavelist[0].Obytes; /* store input bytes in master record */
}

Expand Down Expand Up @@ -1428,7 +1438,7 @@ int ecx_config_overlap_map_group(ecx_contextt *context, void *pIOmap, uint8 grou

/* Find mappings and program syncmanagers */
ecx_config_find_mappings(context, group);

/* do IO mapping of slave and program FMMUs */
for (slave = 1; slave <= *(context->slavecount); slave++)
{
Expand All @@ -1440,8 +1450,8 @@ int ecx_config_overlap_map_group(ecx_contextt *context, void *pIOmap, uint8 grou
/* create output mapping */
if (context->slavelist[slave].Obits)
{
ecx_config_create_output_mappings(context, pIOmap, group,

ecx_config_create_output_mappings(context, pIOmap, group,
slave, &soLogAddr, &BitPos);
if (BitPos)
{
Expand All @@ -1453,7 +1463,7 @@ int ecx_config_overlap_map_group(ecx_contextt *context, void *pIOmap, uint8 grou
/* create input mapping */
if (context->slavelist[slave].Ibits)
{
ecx_config_create_input_mappings(context, pIOmap, group,
ecx_config_create_input_mappings(context, pIOmap, group,
slave, &siLogAddr, &BitPos);
if (BitPos)
{
Expand All @@ -1472,8 +1482,13 @@ int ecx_config_overlap_map_group(ecx_contextt *context, void *pIOmap, uint8 grou
if (currentsegment < (EC_MAXIOSEGMENTS - 1))
{
currentsegment++;
segmentsize = diff;
}
while (diff > (EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM) && currentsegment < (EC_MAXIOSEGMENTS - 1))
{
context->grouplist[group].IOsegment[currentsegment++] = (EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM);
diff -= (EC_MAXLRWDATA - EC_FIRSTDCDATAGRAM);
}
segmentsize = diff;
}
else
{
Expand Down Expand Up @@ -1526,7 +1541,7 @@ int ecx_config_overlap_map_group(ecx_contextt *context, void *pIOmap, uint8 grou
{
/* store output bytes in master record */
context->slavelist[0].outputs = pIOmap;
context->slavelist[0].Obytes = soLogAddr - context->grouplist[group].logstartaddr;
context->slavelist[0].Obytes = soLogAddr - context->grouplist[group].logstartaddr;
context->slavelist[0].inputs = (uint8 *)pIOmap + context->slavelist[0].Obytes;
context->slavelist[0].Ibytes = siLogAddr - context->grouplist[group].logstartaddr;
}
Expand Down Expand Up @@ -1647,7 +1662,7 @@ int ecx_reconfig_slave(ecx_contextt *context, uint16 slave, int timeout)
if (context->slavelist[slave].PO2SOconfigx) /* only if registered */
{
context->slavelist[slave].PO2SOconfigx(context, slave);
}
}
ecx_FPWRw(context->port, configadr, ECT_REG_ALCTL, htoes(EC_STATE_SAFE_OP) , timeout); /* set safeop status */
state = ecx_statecheck(context, slave, EC_STATE_SAFE_OP, EC_TIMEOUTSTATE); /* check state change safe-op */
/* program configured FMMU */
Expand Down
Loading