Skip to content

Instance Datablocks declarations are now read from the Function Block #72

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

Merged
merged 4 commits into from
Jul 18, 2017
Merged

Instance Datablocks declarations are now read from the Function Block #72

merged 4 commits into from
Jul 18, 2017

Conversation

StefanHasensperling
Copy link
Contributor

This is to make the behavior consistent with Step7.
Also reorders the class a little bit and added comments.

The behavior is fixed, and not yet adjustable via options. The important change is all in "private tmpBlock GetBlockBytes(ProjectBlockInfo blkInfo)"

…. This is to make the behaviour consitent with Step7.

Also added Some comments to the Code
@@ -323,8 +319,9 @@ private tmpBlock GetBlockBytes(ProjectBlockInfo blkInfo)
{
//DB Structure in Plain Text (Structure and StartValues!)
if (mc5code != null)
myTmpBlk.blkinterface =
Project.ProjectEncoding.GetString(mc5code);
if (!myTmpBlk.IsInstanceDB ) //only take interface if the block is not an instance DB. If it is, then reather take the interface from the FB declaration
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do not overwrite the DB declartion if it was already obtained from the Function Block

//The reason is that if you change the comment in an FB, the DB data is not actualized and my contain outdated
//Declarations. When you change the interface, Step7 tells you to "regenerate" the instance DB which only then would
//Actualize the comments. Simple Commentary changes do not change the Datablocks row.
tmpBlock InstFB = GetBlockBytes("FB" + myTmpBlk.FBNumber);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Read the Declaration from the Function block, and overwrite the one read from the S7 Project files

Copy link
Member

@jogibear9988 jogibear9988 left a comment

Choose a reason for hiding this comment

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

Have you tested if actual values will work if the interface in the fb is increased or reduced (without recreating the db).

Copy link
Member

@jogibear9988 jogibear9988 left a comment

Choose a reason for hiding this comment

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

And maybe you could add somth. to the S7ConvertOptions to use the old behavior?

@StefanHasensperling
Copy link
Contributor Author

no, I did not test with actual values. Did not think about this. I will look into this later this week, when i have some time.
But its a good question, what does Step7 do in this case? If i remember correctly, it forces you to recreate and download the DB, loosing the Current values.

@StefanHasensperling
Copy link
Contributor Author

Maybe we can make it like this: by default use the FB interface, except explicitly chosen from the options (same as Step7). And if there is a Mismatch in the declaration (structure, not symbols or comments) between the FB and the DB, then use the DB.

@jogibear9988
Copy link
Member

It's ok for me if we do it so. But before merge we should check if/how it works with the actual values.
Atm. I've also no time to test, so I'll hope you find some time to test.

Thanks for your work you've done already

@StefanHasensperling
Copy link
Contributor Author

I Implemented everything optional (via ConvertOptions). Also I added an Structure Comparison into the "Parameter" Class, which compares the actual structure of Interfaces and Datablocks for compatibility.

The Instance DB now only take the declaration from the FB, if these are compatible. This Check is done by an actual Structure compare. Maybe this could have been done by just comparing the InterfaceChange Date, but i decided to implement it this way.

I did only basic testing with an simplistic project and Function block, but think some more testing needs to be done. Unfortunately i will not have time this week for anymore testing. Maybe somebody can prepare test cases.....

I also added some more comments on various places, ...

Copy link
Member

@jogibear9988 jogibear9988 left a comment

Choose a reason for hiding this comment

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

What should we do if the Interface is not compatible? Is there a way to inform the user, or should we throw a exception?

@jogibear9988
Copy link
Member

Should I merge now?

@StefanHasensperling
Copy link
Contributor Author

Sorry for the delay. Did not have time yet for in depth tests, specifically with DB current data.
I think it should not be an problem with the changes I made, because if there is an Structural difference between the FB and iDB, then the lib is giving preference to the iDB. So in theory it should behave as usual.

But I would like to do some testing just to be sure. I think during this week i will have some time for testing.

@jogibear9988
Copy link
Member

thanks

@jogibear9988
Copy link
Member

i'll merge after your tests

@StefanHasensperling
Copy link
Contributor Author

I tested it with the samples provided in the Issue that was raised. Everything is working fine.
If the DB and FB are compatible, the FB interface is shown, if they are not compatible, the interface of the DB is shown.

I dont know how to test the issue that you mentioned with the acutal data of the datablock. I think it should not be an problem, because Dataintegrity should always be ensured. If DB and FB are compatible, then so are the Actual data. If they are not compatible, then the DB is used as interface, for which the Actual data is compatible anyway.

@jogibear9988 jogibear9988 merged commit 58f1c7e into dotnetprojects:master Jul 18, 2017
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