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

ProcessHeader(string package) - Default revision #7

Closed
yanick76 opened this issue Jun 20, 2019 · 13 comments
Closed

ProcessHeader(string package) - Default revision #7

yanick76 opened this issue Jun 20, 2019 · 13 comments
Assignees

Comments

@yanick76
Copy link

Some messages from some controllers doesn't contains revision, for example MID 0015:
00420015____________0022017-10-03:14:12:30
In this case message parsing fails because MID revision parsed as 0 and DataField doesn't contains data for revision 0.
Possible fix is to change default revision to 1 (Mid.cs:100)
Revision = (package.Length >= 11 && !string.IsNullOrWhiteSpace(package.Substring(8, 3))) ? Convert.ToInt32(package.Substring(8, 3)) : 1,

@Rickedb
Copy link
Owner

Rickedb commented Jun 21, 2019

As I've seen, even when parsing revision equals to 0, it does change it to rev 1 when parsing the "body" of the package, like in ProcessDataFields()

protected virtual void ProcessDataFields(string package)
        {
            if (!RevisionsByFields.Any())
                return;

            int revision = HeaderData.Revision > 0 ? HeaderData.Revision : 1; //This part
            for (int i = 1; i <= revision; i++)
                ProcessDataFields(RevisionsByFields[i], package);
        }

Can you provide an example of a package that is failing?

@yanick76
Copy link
Author

Example package is in my comment it's MID 0015. I forgot to mention it's necessary to replace underscores (_) with spaces. In comment preview extra spaces were removed so I replaced them with underscores.

@Rickedb
Copy link
Owner

Rickedb commented Jun 21, 2019

I'm fixing it on mid 0015 override.
Since making changes on MID class will change every Mid behavior (which is more likely to cause problems/bugs for people).

By now, doing this it will help you out with this problem and I will also study the possibility to modify it to every mid.

@Rickedb
Copy link
Owner

Rickedb commented Jun 21, 2019

Already published on nuget gallery, please update it and close the issue once you check it solved!

@yanick76
Copy link
Author

As far as I understand open protocol revision 0 doesn't exist. In this case it's about way to parse received message from controller and message with revision 0 is always necessary to parse based on revision 1 format.
Anyway, your solution is ok :)

@yanick76 yanick76 reopened this Sep 15, 2020
@yanick76
Copy link
Author

I found similar issue in MID 0061 (version 3.2.0)
I switched parameters in Pack method by mistake and I substribed to tightening revision 0.
Tightening data as result:

02310061            010109020203XXXXXXXXXXXXXXXXXXXXXXXXX04XXXXXXXXXXXXXXXXXXXXXXXXX050006001070000080000091101111120008501300092014000870150008731600000170013018000051900040202020-09-15:04:43:09212017-03-01:08:43:16222230004720285

I fixed it by modify Mid0061.CS like this

        protected override void ProcessDataFields(string package)
        {
            if (HeaderData.Revision == 0 || HeaderData.Revision == 1 || HeaderData.Revision == 999)
            {
                int revision = HeaderData.Revision > 0 ? HeaderData.Revision : 1;
                ProcessDataFields(RevisionsByFields[revision], package);
            }
            else
            (...)

@yanick76
Copy link
Author

Offtopic but I don't want to open new issue for typo in enum file :)
You have typo in file TorqueValuesUnit.cs, value number 3 is LBF_LN but correct should be LBF_IN like Pound-force inch

@Rickedb
Copy link
Owner

Rickedb commented Sep 17, 2020

There are some weird controllers out there, usually the default revision is always 1, but they send empty sometimes.
Anyway, I really need to think about handling this behaviour in the next versions, it might be more common than I imagine.

Also, for your code, better use if(HeaderData.Revision < 2 || HeaderData.Revision == 999), better doing 2 operations than 3.

@Rickedb
Copy link
Owner

Rickedb commented Sep 17, 2020

About the enum, I'm going to change it. However it's may cause break changes.
Thanks for submitting both issues

@yanick76
Copy link
Author

I'm not sure that was probably PF 3000 with controller sw version 10.5 SR4. I think problem was with tightening subscription (MID 0060). I subscribed to tighttening data with revision 0, by mistake :)
And about my code, sorry i was bit tired. I agree your version is better

@Rickedb
Copy link
Owner

Rickedb commented Sep 17, 2020

Makes sense! Anyway, better handle it in the lib too.

Oh, and you don't need to excuse for that, it was just a clue. Also, don't push yourself too much!

yanick76 added a commit to yanick76/OpenProtocolInterpreter that referenced this issue Oct 21, 2020
- Added basic Mode support (MID 2600 - 2606), unfinished some Mids are only placeholders
- Multispindle related fixes
- Added revision 2 support to MID 0042 (disable tool) and MID 0043 (enable tool)
@ekdahl
Copy link

ekdahl commented Feb 8, 2021

Please note that the specification says:
"If the initial MID Revision (revision 1) is required there is three different ways to get it, either send three spaces or 000 or 001."
I.e. there is no revision 0.
1 is the first revision.

@Rickedb Rickedb self-assigned this Feb 10, 2021
@Rickedb
Copy link
Owner

Rickedb commented Jun 6, 2022

Enhancement made at version 5.0.0, please check release notes: https://github.com/Rickedb/OpenProtocolInterpreter/releases/tag/5.0.0

@Rickedb Rickedb closed this as completed Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants