Simultaneous Client + Server Decoding

[david-vankampen]

replaced master with client, slave with server, and added support for attempting to analyze both client and server from the same channel. removed some dead and commented code, and also organized the errors a bit. that can probably be taken a bit further yet.

[Marcus10110]

Thanks @david-vankampen!

A few requests if you have time:

1. Could you break this into 2 PRs, one for renaming, and the other for the combined mode decoding support?
2. Please include some sample captures of the combined mode, if possible.
3. Also, ideally we would support simulation mode for the new combined decoding option. You can enable simulation in Logic 2 by setting the environment variable ENABLE_SIMULATION=1 before launching the software.
4. Lastly, please include links to any documentation / specifications you referenced when making modifications.

Thanks!

[david-vankampen]

@Marcus10110 sounds good. For item 1, I just opened PR #4 . Once that completes, this PR should look much simpler. I will work on getting the other items addressed. Though for item 4, I am not sure what I would link to. Just the modbus spec? This doesn't implement any NEW parsing, just connects the two existing capabilities into one.

[Marcus10110]

Thanks David, I've merged your rename PR. Could you update this one accordingly?
Also sorry for the long delay, I just got back from vacation.

[david-vankampen]

@Marcus10110 I have merged in the updates but something appears to have gotten shuffled slightly off now functionally - it is correctly decoding/framing the client request, but incorrectly starting the server response. I will investigate and update when I have found resolution.

[david-vankampen]

Ah, disregard. 😄 It was a parity bit settings issue. OK so here are a couple screenshots. I have stacked up 4 analyzers - the original client analyzer in yellow, the original server analyzer in red, the new combined client and server analyzer in green, and then, just to show individual bytes, outside of the modbus protocol, the async serial analyzer in tan.

So first, here is the client's request.

you'll notice in this one, the yellow (client only) and green (clioent + server) decoded it the same. the server decoder did not handle it correctly, and in fact it continues its frame into the response.

Next, here is the response, which came ~2ms later:

here you will see zero decoding occurred on the client-only analyzer... the server-only analyzer is carrying forward an error from the previous frame, BUT the client+server analyzer did analyze it correctly. This is supported by the async serial bytes shown below it.

If i trim away the request frame, so the first frame being analyzed is the response:

then the original server analyzer decodes correctly, but neither the client analyzer, nor client+server analyzer, does. The latter does not work because it is set up to expect BOTH messages - request then response, and does not handle an analyze that STARTS with a response.

[Marcus10110]

then the original server analyzer decodes correctly, but neither the client analyzer, nor client+server analyzer, does. The latter does not work because it is set up to expect BOTH messages - request then response, and does not handle an analyze that STARTS with a response.

That's what I was worried about, the original developer had this problem when they first created the decoder, which is why we didn't add support originally. However I never took a close look at trying to determine if a packet is a request or response.

I suggest the following:
Add a new setting interface. A Boolean option, perhaps titled "Assume data begins with response (Client & Server mode only)".

You could use the value of that to seed the value of processingResponse. That way, users would have a way to fix it if they have a decoding error.

Ideally, there would be some way to tell the difference between a request and a response, even if you have to wait till the end of the packet to make the determination.

One way we've dealt with this sort of issue is to buffer the decoded bytes and their time stamps until we can tell what kind of message it is, then decode those stored bytes normally, and continue from there normally. We had a similar problem with the modbus analyzer, where you can't tell the difference between '0' bits and '1' bits until you see a transition - '01' or '10'. At that point, we would commit all of the buffered bits we had decoded.

[Marcus10110]

Nice work!

I only found one new issue from the code review, mentioned in the comments below. Specifically, those new enum values will mess up saved captures and saved presets as written now, but it's an easy fix.

I also have a suggestion about the response first deciding problem in the comments on the PR.

Lastly, could you send us a saved capture that contains both traffic?

[david-vankampen]

@Marcus10110 I believe the most recent commit should address the suggestion for adding a setting for seeding the processingResponse variable with a setting boolean.

with that option, here is a capture with request and response successfully decoded:

and then if I trim that to just the response, and toggle that boolean to true, the response gets successfully decoded on its own:

Regarding

Lastly, could you send us a saved capture that contains both traffic?

how should those be provided?

[Marcus10110]

one small request in the legacy save/load code. Otherwise looks good!

[Marcus10110]

Regarding

Lastly, could you send us a saved capture that contains both traffic?

how should those be provided?

Please create a new folder in the repo called "samples", then add the *.sal file, and a new readme.md file that has a quick description of the file. Thanks!

[david-vankampen]

@Marcus10110 all comments should be addressed. Thank you for your feedback.

[Marcus10110]

Looking good! Sorry there always seems to be "one more thing"

I've tested your analyzer locally, looking good. Thanks for the files you've included!

Two things:

1. (most important) We're planning to enable simulation support again in the logic 2 software. Right now, you can enable simulation by setting an environment variable before launching logic, ENABLE_SIMULATION=1.

However, when Both mode is selected, the simulator never produces any data, which then in turn prevents the simulation device from generating any data, breaking the capture. This is a pretty easy fix.

If you head over to this function, you will see 2 big if statement cases, one for client, one for server. We just need a 3rd case for both mode. Something like this would work fine, you don't need to make it as thourough as the other two if you don't want to. We heavily lean on simulation when we write analyzers, because it let's us easily test rare features or edge cases that aren't easy to reproduce with real world electronics.

U32 ModbusSimulationDataGenerator::GenerateSimulationData( U64 largest_sample_requested, U32 sample_rate,
                                                           SimulationChannelDescriptor** simulation_channels )

Something like this would probably be good enough. (I haven't tested this)

        if( mSettings->mModbusMode == ModbusAnalyzerEnums::ModbusRTUBoth ||
            mSettings->mModbusMode == ModbusAnalyzerEnums::ModbusASCIIBoth )
        {
            SendGenericRequest( 0x01, 0x01, 0x0013, 0x0013 );
            mModbusSimulationData.Advance( mClockGenerator.AdvanceByTimeS( .125 ) );

            U8 bytes[ 3 ] = { 0xCD, 0x6B, 0x05 };
            SendGenericResponse( 0x01, 0x01, 0x03, bytes );
            mModbusSimulationData.Advance( mClockGenerator.AdvanceByTimeS( .125 ) );
        }

You can learn more about simulating your analyzer here:
https://support.saleae.com/user-guide/using-logic/demo-mode#generating-analyzer-simulation-data

2. This is super minor, but it looks like you had manually re-named the modbus analyzer modbus3, probably in order to prevent it from conflicting with the original modbus analyzer. Unfortunately, this means your sample files were saved using modbus3, and when loaded into the software, the analyzer is automatically removed because no modbus3 analyzer was found.

I'd like to get this fixed. When I'm testing variations of analyzers we ship with, I usually just add an underscore to the end of the original dll before launching the software. Our software automatically loads all dlls in this folder, but only if they have the "dll" extension. If they say end with "dll_", that file will be skipped. That way you can develop your version of modbus with the original modbus name. Of course, this doesn't help if you want to compare side-by-side.
Built in analyzers are found here:
C:\Program Files\Logic\resources\windows\Analyzers

[david-vankampen]

@Marcus10110 I implemented change suggestion 1. Regarding the second item - I am working in Linux, running the AppImage. Do you have a suggestion for manually overriding the default analyzer there?

[Marcus10110]

@Marcus10110 I implemented change suggestion 1. Regarding the second item - I am working in Linux, running the AppImage. Do you have a suggestion for manually overriding the default analyzer there?

Ah yes, that's right. There is one extra step on Linux:

./Logic-2.3.45-master.AppImage --appimage-extract
This will extract the contents to a new directory, squashfs-root. I think the analyzers are located here:
resources/linux/Analyzers
The same basic principle applies, except with .so_. To run, just launch the Logic binary extracted in the squash-fs directly. Let me know if you have any trouble with it.

[david-vankampen]

@Marcus10110 ok - should be all set!

[david-vankampen]

@Marcus10110 one other thing I noticed - the libmodbus_analyzer.so that I am generated was about 3x smaller than the one that was in the extracted appImage:

it appears they are being deployed with debug-info turned on? Not sure if that is intended. Just figured I'd point it out.

[Marcus10110]

@david-vankampen, nice catch. Yes, we include debug information on purpose. You can see that we build the RelWithDebInfo cmake configuration in CI here:

modbus-analyzer/azure-pipelines.yml

Line 37 in 687dfaa

CMAKE_ARGS: '-DCMAKE_BUILD_TYPE=RelWithDebInfo'

The reason is because when the software crashes, if the user has crash reporting enabled, we'll upload the minidump to a crash reporting server. There, we'll create stack traces from the crash.
For our main code, graph_server_shared.so, we strip the symbols, but also upload the symbols to our crash reporting server so it can symbolicate crashes. However for analyzers, my hope was that we could skip that extra step by leaving symbols in the builds. However I don't recall if it was working or not. Crashes from inside analyzer code are pretty rare, so it hasn't been a priority for me to check.
Since our analyzers are open source, it doesn't make much difference for us if symbols are stripped or not.

[Marcus10110]

I'll get through another review as soon as I can, but I'm backed up with other work for the moment. If it all looks good, I will go ahead and merge the PR for you too.

[david-vankampen]

@Marcus10110 whats the status on this?

[david-vankampen]

@Marcus10110 @schmicro bumping this PR back up - what is the status here?

[Marcus10110]

Sorry for the Year long delay!
At the time, I had run into some problem when testing your modifications, but never followed up. I'm very sorry about that.

I just started testing this now with the samples you provided and the simulator, and it looks like the simulation for the new mode isn't working properly. Here is a screenshot:

However, I was sure I was having another issue which I can't locate now. I'll keep looking, but at the moment let's assume simulation is the last issue.

Also this would need to be updated from our branch, I think it's slightly out of date. If you would like help with any of this please let me know.

Also to help make sure this doesn't fall off of our radar again, please open a support ticket to go along with this - we don't have a good system for tracking open github issues at the moment, but our support system is much better.

[david-vankampen]

@Marcus10110 dusting things off here - I've synced to upstream master, and am working to replicate the results you're reporting. To be clear, the issue is on RTU C&S, right? I am thinking the single clients, and ASCII C&S, look correct?

[Marcus10110]

To be clear, the issue is on RTU C&S, right? I am thinking the single clients, and ASCII C&S, look correct?

That's correct

[ntik98]

plz can you give detail guide how to show this menu too logic2. i have created dll file . what next?

[timreyes]

@ntik98 Once you've got your .dll file generated, you can import your custom analyzer into the software via the instructions below.
https://support.saleae.com/faq/technical-faq/setting-up-developer-directory

[GunasekaranMP]

@Marcus10110 @timreyes I'm looking for this same plugin for the Modbus RTU serial protocol and this will be very useful. I request please approve the above changes and please share the .dll file.

[sebistark]

Everything has been implemented. Can anybody merge the pull request? or approve the review? @Marcus10110 ?

[timreyes]

@GunasekaranMP @sebistark Our entire team is currently heads down on some high priority projects at the moment. We’re likely unable to commit resources anytime soon to review and merge the PR into our production software. However, we’d love to get back to reviewing all Issues and PRs submitted for our analyzers as soon as we are available.

Having said that, the latest commit can be found below, along with their provided analyzer lib files in case you would like to test it with your software.

• GitHub - saleae/modbus-analyzer at 116851c1ba15e3722992995393646b77629812fa
• Release v0.0.7 · saleae/modbus-analyzer · GitHub