search

OpenEtherCATsociety / SOES

Simple Open Source EtherCAT Slave
Other
578 stars 249 forks source link

Feature/simplify foe #64

Closed elsp1991 closed 3 years ago

elsp1991 commented 5 years ago

attempting to fix some of the FOE implementation issues

elsp1991 commented 5 years ago

So I would like to be able to trigger FOE_abort from the application layer. Do you think that is ok to expose the function itself or should we do a more complicated solution. With a first try was working nicely. The Abort function will also reinitilize FOE by calling FOE_init so is not leaving it in a dirty state.

elsp1991 commented 5 years ago

I figured out that I dont need to expose FOE_abort as the error should be better handled by the FOE library. Searching the documentation of etherCAT could not found a document that really defines the FOE error codes and when to be thrown. If someone can reference it would be helpful. Please check the "add password and state check" commit c2b703c from #64 https://www.ethercat.org/memberarea/download/ETG5003_2_V1i0i0_S_D_FwUpdate.pdf Provides poor documentation for this part

nakarlsson commented 4 years ago

@elsp1991 , we're about to add some other changes having impact on the API. Can this PR still be commit ASIS?

elsp1991 commented 4 years ago

Yes this PR includes only obvious fixes and is not dealing with the point I was mentioning here https://github.com/OpenEtherCATsociety/SOES/issues/63 It can be merged and was working for my setup

nakarlsson commented 4 years ago

@elsp1991 What do you think of , bullt 2 is to minimize need of future API changes. Can be combined in to 2x uint16_t or single uint32_t.

  1. Rename foe_writefile_cfg_t to foe_file_cfg_t
  2. Use the padding:24 in some way? instead of uint8_t write_only_in_boot; having uint8_t write_state_permission; uint8_t write_flags; uint8_t read_state_permission; uint8_t read_flags;
elsp1991 commented 4 years ago

@nakarlsson observing the error codes in page 92 of the ETG1000.6 , it is implied that a file can be accessed either in any state or only in boot state or all states except of boot . There is no error code for example WRONG_STATE to allow arbitrary state permissions and that is why I did not implemented it.

elsp1991 commented 4 years ago

Also page 92 of ETG1000.5 Describes a FoE struct implementation but it looks like is missing some information as the documentation of the arguments is not consistant

nakarlsson commented 4 years ago

How about

  1. Rename foe_writefile_cfg_t to foe_file_cfg_t

  2. Just thinking out of the box, that a file might have different read/write properties. the flags is forth future use. uint8_t write_boot_only; uint8_t write_flags; uint8_t read_boot_only; uint8_t read_flags;

nakarlsson commented 4 years ago

@elsp1991 , lets wait with the read- and flags stuff and keep the padding:24.

Any thoughts on going for foe_file_cfg_t instead of foe_writefile_cfg_t. if I recall we didn't see a need for a foe_readfile_cfg, than foe_file_cfg_t would be able to cover "any" file.

elsp1991 commented 4 years ago

I dont see any problem

nakarlsson commented 4 years ago

Could you update the PR going for foe_file_cfg_t?

nakarlsson commented 4 years ago

@elsp1991 , can you update the PR with foe_file_cfg_t insted of foe_writefile_cfg_t

nakarlsson commented 3 years ago

We've added this PR as part of https://github.com/OpenEtherCATsociety/SOES/pull/86

Can we close this? You can revert the commits and rebase to master?

nakarlsson commented 3 years ago

Close since it was added as part of another PR