[WIP] feat(plc4net): revive the .NET port — net8.0, SPI3 runtime, pure-.NET mspec generator - #2656
TomNewChao wants to merge 48 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Revives the plc4net (.NET) port by modernizing it to .NET 8, restoring test execution, aligning key API/SPI surfaces with PLC4X SPI3 concepts, and adding a foundational driver runtime plus a TCP transport implementation.
Changes:
- Retarget projects to
net8.0and centralize shared build/package properties viaDirectory.Build.props. - Fix the PLC value model’s virtual dispatch and add a bit-level codec (
BitReader/BitWriter) + repairedReadBuffer/WriteBuffer. - Add SPI3-aligned runtime building blocks (connection-string parsing, driver/connection bases, message codec) and a TCP transport with CI workflow coverage.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| plc4net/transports/tcp/TcpTransportInstance.cs | Adds async TCP transport instance with background read loop + ring buffer. |
| plc4net/transports/tcp/TcpTransportConfiguration.cs | Defines TCP transport configuration defaults and options. |
| plc4net/transports/tcp/TcpTransport.cs | Implements TCP transport factory + address parsing and option parsing. |
| plc4net/transports/tcp/plc4net-transport-tcp.csproj | Introduces TCP transport project. |
| plc4net/spi/spi/transports/TransportException.cs | Adds transport-specific exception type. |
| plc4net/spi/spi/transports/RingBuffer.cs | Adds fixed-capacity ring buffer used by transports/codecs. |
| plc4net/spi/spi/transports/ITransportInstance.cs | Introduces transport instance contracts (sync + async listener variant). |
| plc4net/spi/spi/transports/ITransport.cs | Introduces transport factory + transport manager registry. |
| plc4net/spi/spi/transports/BaseTransportInstance.cs | Adds base transport instance with config + driver-config handling and Dispose. |
| plc4net/spi/spi/model/values/PlcWSTRING.cs | Fixes string value dispatch/exposure. |
| plc4net/spi/spi/model/values/PlcWORD.cs | Fixes overridden bit-accessors for WORD. |
| plc4net/spi/spi/model/values/PlcWCHAR.cs | Fixes string dispatch/exposure for WCHAR. |
| plc4net/spi/spi/model/values/PlcValueAdapter.cs | Makes IPlcValue API virtual to enable correct overriding/dispatch. |
| plc4net/spi/spi/model/values/PlcSTRING.cs | Fixes string dispatch/exposure. |
| plc4net/spi/spi/model/values/PlcSimpleValueAdapter.cs | Fixes overriding for “simple value” classification. |
| plc4net/spi/spi/model/values/PlcSimpleNumericValueAdapter.cs | Fixes numeric conversions/range checks and interface dispatch. |
| plc4net/spi/spi/model/values/PlcLWORD.cs | Fixes overridden bit-accessors for LWORD. |
| plc4net/spi/spi/model/values/PlcDWORD.cs | Fixes overridden bit-accessors for DWORD. |
| plc4net/spi/spi/model/values/PlcCHAR.cs | Fixes string dispatch/exposure for CHAR. |
| plc4net/spi/spi/model/values/PlcBYTE.cs | Fixes overridden bit-accessors for BYTE. |
| plc4net/spi/spi/model/values/PlcBOOL.cs | Fixes BOOL accessor dispatch and adds conversions. |
| plc4net/spi/spi/generation/WriteBuffer.cs | Reworks write buffer to use in-house bit writer + fixes float/string/array writing. |
| plc4net/spi/spi/generation/ReadBuffer.cs | Reworks read buffer to use in-house bit reader + fixes numeric/string/array reading. |
| plc4net/spi/spi/generation/ParseException.cs | Makes ParseException a real Exception type. |
| plc4net/spi/spi/generation/BitWriter.cs | Adds MSB-first bit writer. |
| plc4net/spi/spi/generation/BitReader.cs | Adds MSB-first bit reader. |
| plc4net/spi/spi/drivers/MessageCodecBase.cs | Adds SPI3-like message codec base and IMessage contract. |
| plc4net/spi/spi/drivers/DriverBase.cs | Adds SPI3-like driver base (transport resolution + connection creation). |
| plc4net/spi/spi/drivers/ConnectionBase.cs | Adds SPI3-like connection base wrapping a transport instance. |
| plc4net/spi/plc4net-spi.csproj | Removes net45-only dependency and aligns packaging with shared props. |
| plc4net/spi-test/test/transports/TcpTransportAddressTests.cs | Adds tests for TCP transport address parsing. |
| plc4net/spi-test/test/transports/RingBufferTests.cs | Adds ring buffer unit tests. |
| plc4net/spi-test/test/model/values/PlcValueTests.cs | Adds interface-dispatch-focused value model tests. |
| plc4net/spi-test/test/generation/BufferTests.cs | Adds codec round-trip tests for bit reader/writer and buffers. |
| plc4net/spi-test/test/drivers/DriverBaseTests.cs | Adds driver-base/transport-selection tests. |
| plc4net/spi-test/test/drivers/ConnectionStringTests.cs | Adds tests for SPI3-aligned connection string parsing + secret redaction. |
| plc4net/spi-test/plc4net-spi-test.csproj | Adds dedicated SPI test project with proper test SDK refs. |
| plc4net/plc4net.sln | Updates solution to include new test + transport projects and platforms. |
| plc4net/drivers/knxnetip/plc4net-driver-knxproj.csproj | Updates KNX driver project dependencies (e.g., NLog). |
| plc4net/drivers/knxnetip-test/test/knxnetip/readwrite/model/KnxDatapointTests.cs | Fixes KNX test vector and asserts float parsing. |
| plc4net/drivers/knxnetip-test/plc4net-driver-knxproj-test.csproj | Ensures test suite actually runs (adds Microsoft.NET.Test.Sdk, marks non-packable). |
| plc4net/Directory.Build.props | Centralizes target framework + shared packaging/build properties. |
| plc4net/api/PlcDriverManager.cs | Refactors driver manager to SPI3-style registry and sync connection creation. |
| plc4net/api/plc4net-api.csproj | Aligns API project with centralized build props. |
| plc4net/api/api/model/IPlcTag.cs | Renames Field→Tag concept for SPI3 alignment. |
| plc4net/api/api/IPlcDriver.cs | Updates driver contract to sync SPI3-like Connect() methods. |
| plc4net/api/api/IPlcConnection.cs | Updates connection contract to sync Close() + tag parsing. |
| plc4net/api/api/ConnectionString.cs | Adds SPI3-aligned connection-string parser + secret redaction. |
| .github/workflows/dotnet-platform.yml | Adds cross-platform CI job for building and running .NET tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Wow ... quite a bit happened here during my few days off :-) So one thing I wanted to point out: Even if in Java several external dependencies existed for various things (such as BitBuffers etc), I still rewrote everything from scratch in SPI3. One the one side, this way I could build a perfect fit for PLC4X, but also I thought this would be a perfect template for porting to other languages as there is no need to fins a "suitable but mostly not 100% replacement". Another thing I noticed: We currently rely on the plc4x build-tools maven plugin to generate code. In my commercial offering I decided to give something else a try: I built an mspec parser and code-generator that fits the target language (in that case Rust) ... possibly it might be worth investigating the options to use a pure dotnet code generation tool ... the antlr4 grammar for mspec should help a lot with that. In the end a pure dotnet toolchain would eliminate the dependency on Java for dotnet developers while still being able to call the build from the overall maven reactor. I think the API diverged quite a bit since it was created several years ago, but i think I saw you already picked up some of these changes. If you need any help ...don't hesitate pinging me. I can also invite you in the plc4x slack channel, if you want more instant feedback (during sensible EU times ;-) ) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
plc4net/transports/tcp/TcpTransportInstance.cs:1
- On orderly remote shutdown (
bytesRead == 0), the socket is not disposed here. Because_openis set to 0 before any call toClose(), subsequentClose()calls can become no-ops (due to the CAS guard), leaving the socket/resources to finalization. Consider triggering the normal close/dispose path here (e.g., perform the same CAS-based shutdown/dispose sequence used byClose()), or refactorClose()to always dispose the socket even if_openis already 0.
plc4net/transports/tcp/TcpTransportInstance.cs:1 - Prefer
Array.Empty<byte>()overnew byte[0]to avoid an unnecessary allocation and follow common .NET conventions for empty arrays.
Thanks for the pointers — the code generation one lands on the open question in the description, and with an option I hadn't considered. The bit buffers were the same call in miniature: I dropped Ayx.BitIO and wrote the reader/writer rather than hunting for a closer package. That you rewrote that layer in SPI3 for the same reason is useful — I'll take SPI3 as the reference to follow rather than a source to copy line by line, and say so where .NET pushes a different shape. On a pure .NET toolchain: agreed, and for the reason you give. Removing the Java dependency for .NET developers is worth more than finishing the freemarker templates. I had a look at the antlr4 grammar and the parser side does look straightforward — the work sits above it, in the type model. Slack sounds like the right place for the rest — yes please, and thanks for the offer. I'm on UTC+8, so your working day runs from my afternoon into my evening; that lands inside sensible hours on both ends. |
|
As it's challenging to get github-user-to-email-addresses ... please send me the address I should send the invite to cdutz@apache.org |
b6bb5ba to
fa0c898
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 56 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
plc4net/transports/tcp/TcpTransportInstance.cs:1
Socket.Send(...)can legally return 0 (e.g., when the connection has been closed), which would make this loop spin forever becauseoffsetnever increases. Consider capturing the return value, and if it is 0, treat it as a connection failure (throwTransportException/ close the connection) to avoid an infinite loop.
plc4net/transports/tcp/TcpTransportInstance.cs:1- When the ring buffer is full, the read loop polls with a 1ms delay. Under sustained backpressure this can cause unnecessary wakeups/CPU usage. Consider replacing this polling with a waitable signal (e.g., a
SemaphoreSlim/AsyncAutoResetEventthat the consumer signals after draining), or at least use a larger/exponential backoff delay to reduce churn.
e285d48 to
cf0302b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 56 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
.github/workflows/dotnet-platform.yml:28
- The path filter
plc4net**is overly broad (it also matches paths that merely start withplc4net, e.g.plc4netfoo/...). Usingplc4net/**is the typical and more precise way to scope to the directory tree.
paths:
- code-generation/**
- protocols/**
- plc4net**
pull_request:
.github/workflows/dotnet-platform.yml:65
actions/setup-javais configured withdistribution: 'adopt', but AdoptOpenJDK has been superseded by Eclipse Temurin and may stop being supported/updated. Switching totemurinkeeps the workflow on a maintained JDK distribution.
distribution: 'adopt'
plc4net/spi/spi/transports/RingBuffer.cs:140
- The comment says a single subtraction replaces a modulo, but the implementation uses a modulo. Either update the comment or change the implementation so the documentation matches the behavior.
plc4net/spi/spi/generation/ReadBuffer.cs:61 - HasMore currently returns true for negative bitLength values, which is nonsensical and can mask caller bugs. Consider rejecting negative sizes explicitly.
plc4net/transports/tcp/TcpTransport.cs:56 - receive-buffer-size can be set to 0 (or negative) via the connection string, which then crashes TcpTransportInstance when constructing the RingBuffer (capacity must be positive). Consider treating non-positive values as invalid and falling back to the default here.
|
@chrisdutz should that be part of 1.0.0? |
|
Well we shouldn't postpone the release too long. If something usable is done soon, sure. Otherwise the next release could be done any time. Is actually a quite streamlined process (as long as it's part of the monorepo. |
|
Also ... as this is considered a significant contribution .... before we can merge this you would need to file an ICLA with apache: https://www.apache.org/licenses/icla.pdf ... if you are doing this work as part of your day-job you should also consider your company signing a CCLA https://www.apache.org/licenses/cla-corporate.pdf |
|
Now that I had the time to catch up with everything else after my holiday yesterday, now I had the time to thorougly read your initial message. So the PLC4Net doesn't have to be identical to the Java version. It was allways our goal to make the libratries feel natural in their normal ecosystem. With Java we're using CompletableFutures, with Go we're using channels ... whatever is the best way to do things in the target language. What we do try to keep, is the general usage pattern: So if connections are synchronous in Java, it would sort of be bad if it wasn't in other languages. Also if the naming of things was kept ... like the usage of "Tag" and "Query" and that you can add a Tag or a TagString, .... Admittedly I don't quite understand what you mean with your first divergence: "ConnectionString is a public type in the API, not the SPI. It has no SPI dependency, and PlcDriverManager — which lives in the API — needs it to route the protocol code." Could you please explain that? I think when I did the first work on PLC4Net, I chose KNX as this used the most of our mspec-functionality ... I thought if I get this driver working, the rest will most probably also work. Usually when starting on a new language I started with Modbus as this is the simplest one of them all for which a test-bench is cheaply available. KNX also had this super odd encoding for 16 bit float which isn't really a full half-precision-iec float. In the new SPI3 java version, I think I used a dedicated float implementation: The implementation is here: However, I built things in SPI3 so theoretically a protocol module could bring along it's own Encoding implementation by putting the class in the driver module and registering it via a org.apache.plc4x.java.spi.buffers.bytebased.encoding.Encoding property file. As you could see I never really got very far with the .Net version as the person that claimed to want to help decided to go away. Usually my process was that I hand wrote some types from Modbus in the target language, then I copied the java templates and started adjusting them till the output matched the hand-written one. the data-io template you found was simply one copied but never touched by me as the work stopped before I had time to do that. As you saw on the discussion list, we're currently considering splitting out Go and also creating go-native code generators. Possibly this is something you would also like to do. Initially the idea was to have one system for generating code. One system anyone would understand and maintaining would therefore be easier. Also is it one thing to understand a programming language, but it's a totally different thing to understand the tooling available in a language, the best practices and how to nicely integrate this into a smoothly running build. For java and maven we knew how to do this, for the others not so much ;-) So the current template+maven approch was simply a quick win that we knew how to do. However, have I found out with my own closed-source work, that there are huge benefits for running this in a tool-native fashion ... then I guess only the resolution of the protocol modules would need to be implemented and the code-gen is built per language. The mspec format is documented in an Antlr4 grammar for which there should be tooling in most languages. The core mspec format is described here: I hope I manged to answer your questions ... if not ... don't hesitate to ask ... ideally here ... if it has to be quickly and you see I'm online in Slack, just ask there. |
Thanks — knowing the goal is to make the libraries feel natural in their normal On the ConnectionString divergence — it does not hold up The three states side by side: (*) = the full connection-string parser (1) and (2) are the same shape. I broke that in (3). What happened: I read "Uri cannot parse the whole s7:cotp://host form" — which 
Java has exactly the same limitation on the two-scheme form and lives with it, Counting the consumers settles which module the type belongs in: api/PlcDriverManager.cs:60 ConnectionString.Parse(..) 1 site <- the line I added Revert that one line and the type has no consumers left in the api module. So it On the general approach My intent throughout has been to follow how the Go and Java modules are used so On KNX I did not choose it, I inherited it — plc4net/drivers/knxnetip/src already On tool-native code generation I agree with building it per language. Each language has its own idioms, tooling Where I would like to go next
One request This PR is really me probing for direction rather than proposing something |
…d transport The Modbus TCP wire model round-trips the shared vectors and the RTU connection had SendAndReceive coverage, but ModbusConnection itself - the MBAP header, the transaction-id correlation, the read loop - was only ever exercised by tools/modbus-verify against real hardware. - Tcp_read_holding_register_returns_the_value: a Read Holding Registers request decoded to the value, with the MBAP frame that went on the wire asserted byte for byte (transaction id 2, length, unit, PDU). - Tcp_write_single_register_returns_OK: a Write Single Register round trip. - Tcp_a_modbus_exception_fails_the_tag_and_the_connection_survives: an ILLEGAL DATA ADDRESS response fails that tag, and the next read on the same connection (transaction id 3) still succeeds. MbapFrame() builds the response frames; the connection numbers transactions from 1 with a pre-increment, so a fresh connection's first request is id 2. 418 tests, 0 warnings.
The packages carried only id, version, authors, license and project URL - no description, readme, repository link or symbols. Enough for a local folder feed, thin for anything a consumer browses. - Directory.Build.props: a shared Description, RepositoryUrl / RepositoryType, and PackageReadmeFile. DebugType=embedded puts the PDB in the assembly so a symbol server is not needed and feeds that reject .snupkg (GitHub Packages) still ship usable symbols. - PACKAGE.md: a short readme packed into every project (a usage snippet, the package map, the pre-release status). - docs/packaging.md: a GitHub Packages section - how a fork publishes a pre-release feed under the same package ids, and the PAT a consumer needs to restore from it. Version references follow the reactor to 1.1.0-SNAPSHOT. The Maven build still overrides the version; nuget.org publishing still waits on the ASF release process. 418 tests, 0 warnings.
The S7 hardware verification ran against a Siemens S7-1214C DC/DC/DC (an
S7-1200-family CPU); the docs said "S7-1200" generically.
- s7-hardware-report.md / s7-hardware-verification.md / testing.md: the
verified device is an S7-1214C. Family-wide statements ("S7-1200 / S7-1500
refuse without PUT/GET") stay generic.
- modbus-hardware-verification.md: the planned Modbus RTU rig is the same
S7-1214C + CM 1241.
- design.md: GAP-5 (S7 hardware verification) is done; the Phase 1 roadmap
notes that items 1-3 and 5 have landed.
The license-header CI job globbed `plc4net/**/*.md` / `**/*.props`, which git ls-files does not expand to files directly under `plc4net/` — so PACKAGE.md (added without a header) and Directory.Build.props were never checked, and apache-rat would have failed the release. - PACKAGE.md gets the standard ASF header block (a comment, so it does not render in the NuGet readme). - The CI job adds `plc4net/*.<ext>` alongside `plc4net/**/*.<ext>` so a file at the module root is covered.
…, PlcBOOL An adversarial review found the value-model fix left three types incomplete, and because PlcValueAdapter answers an un-overridden accessor with `default` rather than throwing, the gaps were silent and invisible to the tests. - PlcNULL: Equals() and GetHashCode() threw NotImplementedException, and IsNull() answered false off the base. It is the fall-through of every generated `DataItem.StaticParse` and of an unresolved lookup, so putting one in a HashSet or comparing two of them crashed. Now: IsNull()/IsNullable() true, all instances equal, stable hash. - PlcBitString (BYTE/WORD/DWORD/LWORD): overrode only the unsigned + bit accessors, so `((IPlcValue) new PlcWORD(0x1234)).GetInt()` returned 0 and GetString() null. Now derives from PlcSimpleValueAdapter (IsSimple() true) and serves the value as a signed short/int/long (range-checked, same as SimpleNumericValueAdapter) and as its decimal string. - PlcBOOL: no numeric view — `.GetInt()` was 0, and GetString() was C#'s "True"/"False". Now 1/0 and "true"/"false", matching plc4j. Regression tests added. 421 tests, 0 warnings.
The review found ModbusConnection raw-polls the transport with no resync: one timeout, and a late response desyncs every following read forever; concurrent Read/Write calls interleave on the wire; and every device exception collapses to InternalError. - PlcResponseCode gains RequestTimeout and Unsupported (Java's PlcResponseCode has both) — a slow device is now distinguishable from a missing one. - SendAndReceive is gated by a per-connection semaphore, so two callers cannot interleave frames. - The receive loop waits for the whole frame (a serial gateway forwards it byte by byte), sanity-checks the MBAP length field, skips a stale response whose transaction id is not ours instead of throwing on it, and resyncs a garbled length by dropping a byte. On a torn read it drains the buffer so the next call starts clean. - A Modbus exception response throws ModbusDriverException carrying the code; MapModbusException turns 0x01/0x02/0x03/0x06 into Unsupported / InvalidAddress / InvalidDatatype / RequestTimeout, mirroring plc4j. - request-timeout connection-string parameter (default 5000 ms). - Write no longer swallows OperationCanceledException. Regression tests: a stale timed-out response no longer bricks the connection; ILLEGAL DATA ADDRESS maps to InvalidAddress. 380 tests, 0 warnings.
…p errors ModbusRtuConnection read "whatever is in the buffer once >= 4 bytes are present": a chunked UART delivery truncated the frame and desynced the connection, and a 2-wire RS-485 transmitter echo (the request bytes, with a valid CRC) passed every check so a read returned the request's start address as an Ok value. - The receive path now computes the exact frame length from the response function code (byte count for a read, fixed for a write, 5 for an exception) and waits for precisely that many bytes. - For a read it first strips an exact echo of the frame it just sent; a write response is byte-identical to its request, so echo-stripping is limited to reads. - Stale bytes are drained before each request and after a torn read or a CRC failure, so one timeout no longer bricks the connection. - A per-connection semaphore serialises callers. - Exception responses map through ModbusConnection.MapModbusException (shared with the TCP path); request-timeout parameter (default 1000 ms). - ParseReadResponse register branch checked pdu.Length >= 3 but read pdu[3]. The two RTU tests asserted only `NotNull`; they now assert the decoded value and use a pump that injects the response after the request, since the driver drains stale bytes before writing. New tests for the echo path and the exception-code mapping. 383 tests, 0 warnings.
…n the loop Follow-ups from the review, none touching the S7 I/O path: - Temporal factories (PlcTIME_OF_DAY / PlcLTIME_OF_DAY / PlcDATE_AND_TIME / PlcDATE_AND_LTIME) validated corrupt segment values with a framework ArgumentOutOfRangeException, which MessageCodecBase.ProcessIncomingData does not catch — a single malformed DT/DTL/TOD frame killed the receive loop. They now throw ParseException, which the codec catches and resyncs on. - S7Tag.Parse used bare int.Parse: a huge number threw OverflowException out of the request builder. Now wrapped as S7DriverException, and the parsed DB number / byte offset / bit offset are range-checked (16/16/3 bits on the wire) rather than silently truncated — %M0.9 and %DB1.DBD70000 are rejected. - KnxNetIpMessageCodec accepted any frame length up to 65535; a spoofed length stalled all inbound processing (heartbeats included) until teardown. Capped at 4096 so it resyncs. - KnxNetIpConnection.Read no longer writes the tag's DPT into the connection's hint map as a side effect — a plain read stopped silently changing how later telegrams for that group address decode. RegisterDatapointHint now throws on an unknown DPT id instead of no-op'ing. - SerialTransportInstance dropped bytes when the ring buffer was full (it pre-clamped the write); it now applies backpressure like the TCP transport, isolates a throwing data listener, and disposes cleanly if Open() fails. Regression tests added. 380 spi-test, 45 knxnetip-test, 0 warnings.
…s, safer enums Generator findings from the review; all three models regenerate deterministically (three consecutive runs are byte-identical) and CI's drift check stays green. - GetLengthInBytes() truncated (`bits / 8`) where plc4j and the generator's own dataIo path round up. A type that is not byte-aligned still occupies a whole trailing byte — now `(bits + 7) / 8`. - ModbusStaticHelper.RtuCrcCheck / AsciiLrcCheck were `params object[]` stubs that threw, so the generated ModbusRtuADU / ModbusAsciiADU could not serialize. A hand-written partial (ModbusStaticHelper.Manual.cs) supplies the real CRC-16 / LRC-8 over address + PDU, ported from plc4j's StaticHelper. - FirstEnumForField<Key>() returned `(T) 0` — not a declared member — for an unmapped key. It now returns `T?` with a `_ => null` arm, and the enum-field read fails with a ParseException rather than carrying a bogus value forward. Known-remaining (documented, not fixed here): two enum constants that share a wire value (S7 TransportSize.COUNTER / DATE_AND_TIME, both 0x1C) still collapse to one C# enum member — a genuine reference-type-enum limitation; and a checksum field is read but not yet *verified* on parse. New test: the RTU / ASCII ADUs serialize without throwing. 381 spi-test.
…rtSize.COUNTER
- EncodeWriteValue encoded by the .NET value's runtime type, so
AddTag("m", "%M0", (int)5) put 4 bytes on the wire for a 1-byte address
item. It now produces exactly tag.DataTypeSize bytes for a scalar,
range-checked, and rejects a REAL written to a non-4-byte tag.
- A %C / %T tag serialised its address item as TransportSize.WORD; the CPU
reads counters and timers with TransportSize.COUNTER alongside the
COUNTERS / TIMERS memory area.
Verified byte-identical for the type-matched writes the S7-1214C run
exercised (WORD 0x1234, BYTE 0x42, BOOL, REAL). 382 spi-test.
…the PDU size The S7 read/write path wrote a request and took the next frame off the wire as its answer, with no correlation and no lock: one timeout desynced every following call, and two concurrent reads could cross responses. - SendAndReceiveS7 wraps the send/receive pair in a per-connection semaphore and loops until it sees a response whose TPDU reference matches the request, skipping a stale frame from an earlier timed-out call rather than returning it. Any failure drains the transport so the next call starts clean. - S7Response carries the TPDU reference (it was discarded before). - A read whose response would exceed the negotiated PDU length now fails every tag with a clear "split the request" message instead of sending a frame the CPU truncates or rejects. (A real multi-PDU split is still a gap.) The test fixtures echoed a fixed reference of 1; they now use the real value (3 for the first op after the handshake). New test: a stale wrong-reference response is skipped. 383 spi-test, 45 knxnetip-test.
S7 and both Modbus connections registered no disconnect listener, so a cable pull / CPU stop / RST surfaced only as a generic timeout on the next request — the SocketException was discarded. - CotpTransportInstance forwards RegisterDisconnectListener to its inner transport (and clears _handshakeDone on a drop, so IsConnected stops lying after a peer COTP/TCP disconnect). - S7Connection, ModbusConnection and ModbusRtuConnection register a listener that logs the causing exception at Warning.
- testing.md: 428 test cases; the Modbus/S7 driver-test descriptions note the new echo-strip, stale-response, TPDU-correlation and write-width coverage. - design.md: GAP-1/2 reworded (RTU now length-framed + de-echoed + resyncing), GAP-3 is now the multi-PDU split, GAP-3b records the two generator limits that remain (wire-value-colliding enum constants; checksum not verified on parse).
…kages flow - <AssemblyVersion> is pinned to 1.0.0.0 (stable across 1.x); the package version and <FileVersion> still track -p:Version. A floating AssemblyVersion meant two packages built at different -p:Version values gave a consumer a load-time binding mismatch (driver-s7 -> plc4net-spi 1.1.0.0, but the packed plc4net-spi.dll was 0.0.1.0). - docs/packaging.md: the GitHub Packages pre-release flow is now a verified procedure (against nuget.pkg.github.com/openIndu) on an independent 0.0.1.x line — build --no-incremental first (a -p:Version change alone does not rebuild), pin AssemblyVersion, override RepositoryUrl to the fork, a gh OAuth token with write:packages is enough, allow for feed-index lag.
|
As i just updated the prerequisite-check script to check for .Net 7 instead of 4.2.1 ... possibly your PR should also ensure that we check for the right version ... also please update the README.md in the root of the project. Here the .Net related section should not be forgotten. |
Thanks for the heads-up. This PR takes plc4net off net452 entirely — the projects now target net8.0 (LTS), which is what finally let the module build on the Linux and macOS CI runners. I've aligned both places you mentioned:
I've also merged current develop and refreshed the PR description, which had fallen behind the branch. BTW, we're verifying S7 and Modbus against an S7-1214C at the moment — should be done within a week. |
Christofer Dutz raised on PR apache#2656 that develop's prerequisite check now looks for the .NET 7 SDK and that the root README's .NET section is stale. The revived port targets net8.0 (LTS), so: - prerequisiteCheck.groovy — checkDotnet() requires 8.0.0 (was 7.0.0 on develop, 4.5.2 before that); the inline comment no longer describes the removed net452 / LangVersion 11 setup. - README.md — the language list drops "abandoned"; the PLC4Net build prerequisites drop the obsolete ".NET Framework 4.5.2 targeting pack / Mono" step and ask for the .NET 8 SDK. - THREAT-MODEL.md, website/.../users/pages/index.adoc — the four spots that quoted the old README wording are synced. The plc4net scope carve-out is unchanged: still out of the model, on the independent ground that it carries no "supported" mark in the protocols index. No source or generated-model change; all three .NET CI checks still pass locally (license headers, generated-code-is-current, 428 tests).
|
I wonder if this could utilize https://docs.github.com/en/pull-requests/how-tos/stacked-pull-requests to seperate the generated code from this PR. This way we had the slim base of just the changes and then a stacked pull request adding the generated code. ATM it is for example not possible to do CoPilot reviews as it just bails due to size |
tools/s7-verify last ran on 2026-09-03, before develop was merged, the prerequisite check moved to the .NET 8 SDK and AssemblyVersion was pinned. Re-ran it against the S7-1214C at 39e3792 to confirm those build and infra commits did not regress the S7 driver. - s7-hardware-report.md — new dated section: PASS 12/12 on two consecutive runs, negotiated PDU 240 bytes, DB100 layout unchanged from 2026-09-03; an added %I0.0 single-read probe (Ok) exercises %I-area addressing outside the data block. Docs only. No source or generated-model change; the S7 driver is untouched since the 2026-09-03 run.
Thanks — the size problem is real. Copilot has refused it since it crossed 20k lines, and 651 files is a lot to put in front of a human. What this PR is right now — an end-to-end revival spike, not a finished contribution. What I'm driving toward is S7 and Modbus working against real hardware: S7 passes on an S7-1214C now, Modbus is next, both within a week I'd expect. Until that path is proven I don't want to freeze the module boundaries, so decomposing now would be guesswork. Once it's proven — split into develop-targeted PRs, each Copilot-sized and reviewable on its own: the SPI3 runtime, then the pure-.NET code generator, then the transports, then one per driver. The generated model classes (424 of the 651 files) travel with their driver — regenerable from mspec, not something to read line by line. |
|
Couldn't you just delete the generated files ... they should be built as part of the built itself, right? And just be carefull to not re-commit them. ... Could add a temp gitIgnore rule. |
Not quite, actually — the build doesn't regenerate the models today. They're committed, and rebuilt only under -Pupdate-generated-code, same as plc4j and the other ports. So dotnet build just compiles what's checked in — there's no generator step to lean on yet. I'm already set on making this reviewable: the plan I gave sruehl splits it into per-driver PRs, and that's what gets each piece under the bot limits. Generating at build instead of committing is a bigger move than the gitignore rule suggests. It turns tools/code-gen into a hard build dependency of every driver — today a generator bug only trips the isolated drift check, not the whole build — and it's the generate-on-build model you moved plc4j away from for reproducible builds. It'd also make plc4net the only port not committing its generated code. There's a real case for it — the drift-check job and the noisy regenerated diffs on generator PRs only exist because the files are committed. I'll start the per-driver split regardless — I'd rather take up build-time generation separately once the split's landed, if it's still needed then. Given the scope and the reproducibility point, I'd like to hear more of your thinking here before deciding. |
Running tools/modbus-verify against a scripted Modbus/TCP slave (ahead of the real S7-1214C run) surfaced two bugs, neither hardware-specific: - ModbusConnection.Read (TCP) only handled Coil and HoldingRegister; DiscreteInput and InputRegister fell through to the default AccessDenied branch without ever reaching the wire. ModbusRtuConnection already handled all four tag types - the TCP connection was the incomplete one. Added the missing cases, mirroring ModbusRtuConnection's BuildReadPdu / ParseReadResponse. - modbus-verify's PrintValue probed IPlcValue.IsBool() first, which plc4net's value model answers true for every numeric adapter too (matching plc4j's PlcValue coercion semantics) - so a holding-register read that correctly produced PlcUINT(0x1000) printed as "True (BOOL)". The value was right, the report was wrong. Now renders by the Modbus tag type actually read, the way s7-verify already does, instead of probing the value. ModbusDriverTests gets two new TCP round-trip tests pinning the input-register and discrete-input wire format and decoding. 430/430 tests pass (was 428, +2). Re-verified end-to-end against a scripted Modbus/TCP slave: holding/input registers report their UINT16 value, coil/discrete their BOOL, an out-of-range read still maps to InvalidAddress.
|
are those then stacked PRs? |
Not the GitHub stacked-PR mechanism, no — each targets develop directly, opened one after the previous merges. They're dependency-ordered either way (each needs the last one's code to compile), so a formal stack wouldn't buy much here; it mainly helps when multiple people need to review different layers in parallel, which isn't the situation with one contributor. Happy to switch to literal stacking if you'd rather review them without waiting on each merge. |
|
Quick update: physical Modbus RTU verification is currently blocked by the RS-485 hardware setup. An independent raw-serial test sends a valid request, but receives no response either, so there is currently no evidence that this is a plc4net driver issue. I still need a known-good USB-to-RS485 adapter or an oscilloscope to isolate the converter from the CM1241. My proposal is to keep this limitation documented and proceed with the smaller, dependency-ordered PRs. Physical RTU verification can follow separately once the hardware link is confirmed. |
Signed-off-by: TomNewChao <chaotomzhu@gmail.com>
Signed-off-by: TomNewChao <chaotomzhu@gmail.com>
Signed-off-by: TomNewChao <chaotomzhu@gmail.com>
Summary
This PR revives
plc4netas a buildable and testable .NET 8 implementation ofthe PLC4X API/SPI. It replaces the abandoned
net452build, aligns the runtimewith SPI3, adds a pure-.NET
.mspec→ C# generator, and provides workingModbus, S7 and KNXnet/IP drivers with generated wire models.
It targets
develop/1.1.0-SNAPSHOT, relates to #2655, and follows thedev@discussion from 2026-07-24.What is included
net8.0; shared strict build properties; Linux, macOS and Windows workflow; license-header and generated-code drift checks..mspecparser and C# emitter; generated Modbus, S7 and KNXnet/IP models are checked into the tree and drift-checked.S7-1214C hardware verification
plc4net/tools/s7-verifywas run through the public driver API against a realSiemens S7-1214C DC/DC/DC at rack 0 / slot 1. The PLC had no external equipment
attached during the persistent output test.
Verified behavior:
%I0.0/%IB0/%IW0/%ID0,%Q0.0/%QB0/%QW0/%QD0, and%M100.0/%MB100/%MW100/%MD100.DWORD in DB100, M100..M117 and Q0..Q17.
connections: 25/25 passed. Inputs were read-only; DB/M/Q restores were
deliberately disabled so the PLC-side values could be inspected.
NotFoundand the connection remains usable.The hardware run also found and fixed the bare S7 Ack framing issue: ROSCTR
0x02, like AckData0x03, carries the two-byte header error. Both are nowframed as 12-byte headers and mapped to
PlcResponseCodewithout desynchronizingthe following response.
Detailed procedures and immutable run records:
plc4net/docs/s7-hardware-verification.mdplc4net/docs/s7-hardware-report.mdPLC-side visual evidence is checked in under
plc4net/docs/images/:s7-1214c-test-rig.jpg— the isolated S7-1214C DC/DC/DC test bench.
s7-db100-online-values.png— DB100 actual values monitored in TIA Portal.s7-iqm-watch-table.png— I/Q/M absolute addresses and their monitoredvalues after the persistent write.
DB100 values monitored in TIA Portal
I/Q/M absolute-address watch table
Automated verification
dotnet build plc4net/plc4net.sln --no-restore --no-incremental:0 warnings, 0 errors.
dotnet test plc4net/plc4net.sln --no-build: 449/449 passed(404 SPI/driver tests and 45 KNXnet/IP tests).
group read/write and bus monitoring run against a scripted UDP gateway.
including response correlation and protocol error paths.
PackageReferenceconsumer;s7-verifyandmodbus-verifypack as .NETtools.
GitHub Actions has not produced a check run for this non-committer PR; the
workflow currently requires repository authorization. The results above are
local and the hardware results are recorded in the repository.
Review guide
The PR is large because it includes generated protocol models and the
checked-in ANTLR C# parser. A focused review can start with:
.github/workflows/dotnet-platform.ymlandplc4net/Directory.Build.props.plc4net/api/and the hand-written runtime underplc4net/spi/.plc4net/tools/code-gen/excludingsrc/generated/on the first pass.plc4net/drivers/{modbus,s7,knxnetip}/.plc4net/test/,plc4net/docs/design.md, and the hardware reports.The generated model trees under each driver's
src/.../readwrite/model/arereproducible outputs; CI regenerates them and fails on drift.
Deliberate .NET adaptations
and culture-invariant numeric parsing.
ITransportInstanceimplementsIDisposable.async/await.DATE_AND_TIME/ DTLdayOfWeekfollows the Siemens/S7 model conventionused in this codebase.
Known gaps
physical RS-485 verification; the
System.IO.Portswrapper itself has nodedicated hardware-independent unit test.
KNX/IP interface.
split across multiple PDUs.
remains driver-specific.
in
plc4net/docs/design.md, including unused field families, colliding enumwire values and checksum re-verification on parse.
code-generation/language/csFreeMarker backend is no longer consumedby PLC4Net. Whether to remove or separately revive it remains a reviewer/
project decision.
Branch status
6a8ebdae0(feature/plc4net-revival).passed.
developafter recent upstreamactivity. It must be synchronized and reverified before merge; this PR remains
Draft/WIP pending that update and committer review.
Description change log
evidence from 428 tests / S7 12/12 to 449 tests plus the persistent I/Q/M/DB
address and scalar-write matrix. Added the original PLC/TIA evidence images.
No title, Draft state or merge action changed.