search

leducp / KickCAT

A C++ open source EtherCAT master stack
Other
59 stars 13 forks source link

Add mailbox request #144

Closed leducp closed 8 months ago

github-actions[bot] commented 8 months ago

File Coverage Lines Branches
All files 15% 23% 7% :x:
lib/include/kickcat/Error.h 79% 75% 83% :white_check_mark:
lib/include/kickcat/Mailbox.h 86% 87% 85% :white_check_mark:
lib/include/kickcat/protocol.h 61% 61% 0% :x:
lib/include/kickcat/CoE/EsiParser.h 0% 0% 0% :x:
lib/master/include/kickcat/Bus.h 60% 100% 20% :x:
lib/master/include/kickcat/DebugHelpers.h 83% 100% 66% :white_check_mark:
lib/master/include/kickcat/Gateway.h 75% 100% 50% :white_check_mark:
lib/slave/include/kickcat/AbstractESC.h 0% 0% 0% :x:
simulation/EmulatedESC.cc 0% 0% 0% :x:
simulation/EmulatedESC.h 0% 0% 0% :x:
simulation/network_simulator.cc 0% 0% 0% :x:

Minimum allowed coverage is 75%

Generated by :monkey: cobertura-action against 073095d3d457cd169e25e8b4f616929cb8be2b3a

Rdk-T commented 8 months ago

I find the naming around the mailbox a bit confusing.

We have a namespace mailbox, with then the namespaces request/response. In these namespaces we have the struct mailbox and the class mailbox. IMO it would be easier to differentiate them by renaming them explicitly MailboxResponse and MailboxRequest.

There is also the file CoE/Mailbox, which only contains the class SDOMessage, I would suggest to rename the file SDOMessage instead of mailbox.

leducp commented 8 months ago

I find the naming around the mailbox a bit confusing.

We have a namespace mailbox, with then the namespaces request/response. In these namespaces we have the struct mailbox and the class mailbox. IMO it would be easier to differentiate them by renaming them explicitly MailboxResponse and MailboxRequest.

There is also the file CoE/Mailbox, which only contains the class SDOMessage, I would suggest to rename the file SDOMessage instead of mailbox.

I disagree: you almost never use both together and I prefer to use namespace for this purpose (which are designed for that). Note that the old master code needs to be moved in another PR.

Rdk-T commented 8 months ago

I find the naming around the mailbox a bit confusing. We have a namespace mailbox, with then the namespaces request/response. In these namespaces we have the struct mailbox and the class mailbox. IMO it would be easier to differentiate them by renaming them explicitly MailboxResponse and MailboxRequest. There is also the file CoE/Mailbox, which only contains the class SDOMessage, I would suggest to rename the file SDOMessage instead of mailbox.

I disagree: you almost never use both together and I prefer to use namespace for this purpose (which are designed for that). Note that the old master code needs to be moved in another PR.

So the file Mailbox will be split in two ?

leducp commented 8 months ago

I find the naming around the mailbox a bit confusing. We have a namespace mailbox, with then the namespaces request/response. In these namespaces we have the struct mailbox and the class mailbox. IMO it would be easier to differentiate them by renaming them explicitly MailboxResponse and MailboxRequest. There is also the file CoE/Mailbox, which only contains the class SDOMessage, I would suggest to rename the file SDOMessage instead of mailbox.

I disagree: you almost never use both together and I prefer to use namespace for this purpose (which are designed for that). Note that the old master code needs to be moved in another PR.

So the file Mailbox will be split in two ?

I would like to split the mailbox in 2xN -> 2 -> request/resposne N -> services (CoE, FoE, EoE, VoE, and so on)