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

Feature/simplify foe #64

Closed

Conversation

elsp1991
Copy link
Contributor

@elsp1991 elsp1991 commented Aug 1, 2019

attempting to fix some of the FOE implementation issues

@elsp1991
Copy link
Contributor Author

elsp1991 commented Aug 5, 2019

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
Copy link
Contributor Author

elsp1991 commented Aug 5, 2019

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

@elsp1991 elsp1991 marked this pull request as ready for review August 5, 2019 16:07
@nakarlsson
Copy link
Contributor

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

@elsp1991
Copy link
Contributor Author

Yes this PR includes only obvious fixes and is not dealing with the point I was mentioning here
#63
It can be merged and was working for my setup

@nakarlsson
Copy link
Contributor

@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
Copy link
Contributor Author

elsp1991 commented Jun 23, 2020

@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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor Author

elsp1991 commented Jul 1, 2020

I dont see any problem

@nakarlsson
Copy link
Contributor

Could you update the PR going for foe_file_cfg_t?

@nakarlsson
Copy link
Contributor

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

@nakarlsson
Copy link
Contributor

nakarlsson commented Sep 11, 2020

We've added this PR as part of #86

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

@nakarlsson
Copy link
Contributor

Close since it was added as part of another PR

@nakarlsson nakarlsson closed this Oct 5, 2020
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.

2 participants