pcdshub / ads-async Goto Github PK

Test suite is hitting into pytmc changes in TMC handling, specifically from this PR:
pcdshub/pytmc#311 (AMSNETID was actually part of the discussion there 😁 )

Looks like ads-async needs to dig down into the base type for AMSNETID and represent it as a string of bytes. My recollection of my implementation here for this is rather hazy though so fixing the test suite may take a bit.

That said, there's still other stuff to be done:

1. Consider moving these magic numbers into constants
2. Consider switching to a structure for both of these non-AoE (AMS/ADS over Ethernet) packets
   a. I've never seen (well, my recollection again is fuzzy...) this string of bytes before in ADS comms so I'm not sure what to make of it. It seems like an inline/immediate control channel comm of sorts
3. Support this on the server end -> implying to move the support into the 'protocol' section of ads-async
4. Add tests of this to the test suite
5. Support user-specified "local mode" (instead of checking against the target IP of localhost or 127.0.0.1, let the user toggle it on)
6. (Fix the test suite as noted above)

Originally posted by @klauer in #40 (comment)

Current Behavior

• There are two methods TwinCAT PLCs use for communicating using the ADS Protocol: insecure (ADS) and secure (ADS-S, using TLS)
• TwinCAT BSD by default only allows for ads-s through its firewall
• Insecure ADS is only supported is currently only supported in ads-async

Possible Solution

Use Python built-in support for TLS: https://docs.python.org/3/library/ssl.html , add a certificate, and see if TwinCAT will accept our communication.

The following will never raise, even if the PLC disconnects and fails to reconnect:

async for header, _, sample in circuit.enable_log_system():
    ...

Context

Turns out the undocumented things I was discovering with wireshark + TwinCAT may already have existing tools that I was just unaware of!

I made some good guesses on the parameters, but more complete information can be filled in based on these sources.

References

• https://github.com/birkenfeld/ads-rs
• https://github.com/Beckhoff/ADS/blob/master/AdsTool/main.cpp
• https://github.com/Beckhoff/ADS/blob/master/AdsLib/AdsFile.cpp
• etc.

This should be from GitHub now

Expected Behavior

Do not base things on current client/server code until a large refactor goes through.

• The API is a mess, as to be expected for a rushed, incomplete evening side project.
• Client API should be restructured to nicely separate protocol/asyncio implementation
• The end goal should be a user-friendly, extensible client and server API

Current Behavior

You will probably ignore this and continue with what's here.

Possible Solution

Listen to this issue

msgCtrlMask:

This seems consistent with the log messages I see. It should be easier to set log level messages appropriately with the above.

First 4 bytes are an error code.

Using the following:

        add_route_to_plc(
            their_host,
            source_net_id=our_net_id,
            source_name=LOG_DAEMON_HOST,
            route_name=LOG_DAEMON_ROUTE_NAME
        )

with a route name ends up causing adding a route:

Route: LOG_DAEMON_HOST
Net ID: our_net_id
Address: LOG_DAEMON_ROUTE_NAME

Temporary fix is to use route_name=LOG_DAEMON_HOST. Need to look into where this is getting switched up.

Expected Behavior

(pcds-4.1.4) [klauer@psdev04  ~]$ ads-async
 usage: ads-async [-h] [--version] [--log LOG_LEVEL] {get,info,monitor,route} ...

Current Behavior

(pcds-4.1.4) [klauer@psdev04  ~]$ ads-async
 usage: ads [-h] [--version] [--log LOG_LEVEL] {get,info,monitor,route} ...

Possible Solution

Fix the tool to report it correctly.

Messages may be truncated if over 255 characters (actually around 244 or so, as the packet also contains timestamps and other info)

Background

Preface: so long as the client is used with only a single target net ID, there should be no problem using the current implementation.

I had thought I wanted a context manager for the existing client to set a target net_id/port for a given block, but what I think may be missing is a "PLC" - a per-(target net id) with a configurable port. A client can communicate to any number of target net IDs (assuming the routing happens on the target end).

Todo

Need to look more into this one day:

• On the protocol level, tracking of handles and such may be insufficient? (i.e., should it be keyed on (source_addr, handle)?)
• Analogous to the caproto "VirtualCircuit", create a "PLC"/"Connection"/"Circuit" class to represent this layer

Expected Behavior

It should be easy to use the default ADS port without specifying it in a tuple.
Generally speaking, people aren't forwarding ADS over SSH as I do for testing.

Support their_address="host" in addition to their_address=("host", port)

Current Behavior

Requires ("host", port)

Expected Behavior

caproto uses the concept of "ours" and "theirs" to good effect. ads-async should do something similar.

Current Behavior

"server" and "client" are used liberally.

Possible Solution

Rework and rename.
Current users (~0) should expect API thrashing

Context

A logging example provided by Beckhoff proved the viability of an event system workaround, where the following steps could be taken to aggregate all log messages from every production PLC we have:

1. A dedicated host is provisioned
2. Each PLC adds a route to that dedicated host
3. An ADS client (per PLC) is run on that dedicated host
4. Each client enables logging over ADS
5. Those messages are interpreted, converted, and shipped off to logstash

Current Behavior

No client in codebase whatsoever

Expected Behavior

For the purposes of the new logging daemon, a simple client will be required. It will only need to:

• Connect to the server (and reconnect, if required)
• Add/remove notifications (not for symbols, just for group/offset)
• Handle notification callbacks
  • The source of the notification must be known (AmsPort.LOGGER is key here)
  • Be able to upcast the raw notification to a AdsNotificationLogMessage (which we'll define)

Apologies for submitting a false issue as a means to communicate with creators. I'm intrigued about what this tool does. I cloned and installed but can't get it to do 'something'.

Would anyone care to share a bit more info?

this is my output:
INFO:ads_async.bin.utils:Got PLC net ID through service port: BLOCKED.1.1 ERROR:ads_async.protocol:Deserialization of <class 'ads_async.structs.AoEReadResponse'> failed: Buffer size too small (0 instead of at least 8 bytes) bytes=b'' (length=0); this may be fatal. (Header=AmsTcpHeader(reserved=0, length=32) + AoEHeader(target=BLOCKEDa.1.1:851(R0_PLC_TC3), source=BLOCKED.1.1:851(R0_PLC_TC3), command_id=<AdsCommandId.READ_WRITE: 9>, state_flags=<AoEHeaderFlag.ADS_COMMAND|RESPONSE: 5>, length=0, error_code=18, invoke_id=101)) Traceback (most recent call last): File "C:\Users\BLOCKED\AppData\Roaming\Python\Python310\site-packages\ads_async\protocol.py", line 115, in from_wire cmd = cmd_cls.deserialize(view) File "C:\Users\BLOCKED\AppData\Roaming\Python\Python310\site-packages\ads_async\structs.py", line 175, in deserialize new_struct = cls.from_buffer(buf) ValueError: Buffer size too small (0 instead of at least 8 bytes) { "Global_Variables.bBlink": "(Exception) TimeoutError Response not received in 2.0 seconds" }

Supporting PORT_SYSTEMSERVICE would be useful (can add a route by way of UDP, determine AMS NET ID based on just IP address, etc.)