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/rc1 #10

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Feature/rc1 #10

wants to merge 6 commits into from

Conversation

Joshpolansky
Copy link
Member

What:

This is the first release candidate. Looking for feedback on AP, functionality, naming and implementation where relevant. Internals are less relevant at this point and may still change in future versions.

Why:

We want to get this to a releasable version to start making docs and examples.

<TcSmProject xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="http://www.beckhoff.com/schemas/2012/07/TcSmProject" TcSmVersion="1.0" TcVersion="3.1.4024.53">
<Project ProjectGUID="{F6F7C937-218D-4551-B982-43F369B598A4}" Target64Bit="true" ShowHideConfigurations="#x3c7">
<TcSmProject xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="http://www.beckhoff.com/schemas/2012/07/TcSmProject" TcSmVersion="1.0" TcVersion="3.1.4024.55">
<Project ProjectGUID="{F6F7C937-218D-4551-B982-43F369B598A4}" TargetNetId="1.0.0.209.1.1" Target64Bit="true" ShowHideConfigurations="#x3c7">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to remove this NetID?


0:
IF OnEntry() THEN
RequestStep( 1 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we see if STweep will standardize spaces around parameters automatically? This is inconsistent.

END_IF


IF OnSuccess(-1) THEN
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment to explain the significance of -1 would be helpful. Otherwise it may be interpreted as an error state.

In fact, could we make an ENUM for these? OnSuccess(SEQ_COMPLETE), for example?

<TcPlcObject Version="1.1.0.1">
<POU Name="Mock_EM" Id="{2fbe5d7f-06dd-041a-2d03-5fb4cef27abb}" SpecialFunc="None">
<Declaration><![CDATA[
FUNCTION_BLOCK Mock_EM IMPLEMENTS IMOCK_EM
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be the only place where _'s are used in the name. Is there significance to IMOCK being all-caps?

VAR
_MoveUp : PLCopenCommand;
_MoveDown : PLCopenCommand;
_HeightGroup : PLCopenExclusiveGroup := (commands := [_MoveUp, _MoveDown, 18(0)]);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the significance of 18(0) here? I'm surprised to see that as valid syntax, but perhaps I'm missing something.

t : TON;
Up : BOOL;
Down : BOOL;
Up_ : BOOL;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the distinction between Up_ and Up? This could be confusing to new users in the example without a comment.

@@ -89,10 +96,13 @@ TEST_FINISHED();]]></ST>
</Implementation>
</Method>
<Method Name="OnError" Id="{ceefdcc0-adb1-4ad8-928a-8e1e77a9bfee}">
<Declaration><![CDATA[METHOD OnError
<Declaration><![CDATA[{attribute 'no-analysis'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Declaration><![CDATA[{attribute 'no-analysis'}
<Declaration><![CDATA[

'no-analysis' was here twice

]]></Declaration>
<Implementation>
<ST><![CDATA[
IF _MoveDown.Consume THEN
Copy link
Contributor

Choose a reason for hiding this comment

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

Being a user-facing example, this code could benefit from a few brief comments.

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