Skip to content

Parsing message #12

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

Merged
merged 32 commits into from
Feb 9, 2023
Merged

Parsing message #12

merged 32 commits into from
Feb 9, 2023

Conversation

Murmele
Copy link
Contributor

@Murmele Murmele commented Jan 7, 2023

  • parsing messages from vector
  • fixing problem that bigendian is when the flag is zero not one

@Murmele Murmele marked this pull request as ready for review January 7, 2023 18:39
@Murmele Murmele marked this pull request as draft January 7, 2023 18:39
@Murmele Murmele force-pushed the parsingMessage branch 2 times, most recently from dbcd3cb to 13bec82 Compare January 11, 2023 09:20
@Murmele Murmele marked this pull request as ready for review January 12, 2023 16:18
@Murmele Murmele marked this pull request as draft January 19, 2023 12:34
@LinuxDevon
Copy link
Owner

Just getting back from vacation. I see you marked as draft. Are you ready or have other changes coming down the pipeline?

@Murmele
Copy link
Contributor Author

Murmele commented Jan 20, 2023

Hi, I found a big issue in the parsing, this was completely wrong and worked only in specific scenarios. Currently in my hack branch I used a quite performant approach, but is not ready yet.
Not sure what is the best way to get the signals out

@Murmele
Copy link
Contributor Author

Murmele commented Jan 20, 2023

bool Message::parseSignals(const uint8_t* data, int size, std::vector<double>& values) const {
        if (!m_prepared)
            return false;

        if (size > 8)
            return false; // not supported yet

        // Only little endian supported yet!
        // All signals must be little endian
        for (const auto& signal: m_signals) {
            if (signal.is_bigendian)
                return false;
        }

        const uint32_t len = size * 8;

        uint64_t d = 0;
        for (int i=0; i < size; i++) {
            d |= ((uint64_t)data[i]) << i * 8;
        }

        uint64_t v = 0;
        for (const auto& signal: m_signals) {
            const uint32_t shiftLeft = (len - (signal.size + signal.start_bit));
            v = d << shiftLeft;
            v = v >> (shiftLeft + signal.start_bit);
            values.push_back(v * signal.factor + signal.offset);
        }
        return true;
    }

This can be extended for bigendian too. But then it does not support longer once.

Copy link
Owner

@LinuxDevon LinuxDevon left a comment

Choose a reason for hiding this comment

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

Good catch on the endianess! Not sure how i missed that or flipped it... sorry about the headache with that one. Thank you for the work on this one! Here are a few more comments on it.

Not sure completely sure what you meant about this comment since I didn't see it in the PR.

// Only little endian supported yet!
        // All signals must be little endian
        for (const auto& signal: m_signals) {
            if (signal.is_bigendian)
                return false;
        }

Not sure what is the best way to get the signals out

I think you did a good job with this interface to get them out.

@@ -0,0 +1,2 @@
Bitstream reader.
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need this extra library? I was hoping to keep the dependencies for this library down to zero. I guess so long as we can keep it single header which I think we can. If we are using this we might want to pull it in with cmake instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the library from python CAN tools. This are two files, a .c and a .h. I created the cmake file for it to be able to import it to dbc_parser

double min = std::stod(match.str(11));
double max = std::stod(match.str(13));
double factor;
fast_float::from_chars(match.str(7).data(), match.str(7).data() + match.str(7).size(), factor);
Copy link
Owner

Choose a reason for hiding this comment

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

I think i commented on the bitstream but do we need this library as well? If this gets you going I am good with it. I might come back and rework it out if the single header fails with the new libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem is you have to convert the values to a double, but std::stod considers localization. I did not find another way, and this library is way faster than the std lib, and is only a single header

Copy link
Owner

Choose a reason for hiding this comment

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

Ah gotcha. I didn't realize that #8 mattered with this. Learned something new on that.

Comment on lines 29 to 33
/*!
* \brief prepareMessage
* Preparing message to be able to parse signals afterwards. This speeds up parsing
*/
void prepareMessage();
Copy link
Owner

Choose a reason for hiding this comment

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

How does sorting them speed up parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the current approach not. I tried to reimplement the parser like in the python CAN tools, but I was not yet finished. I think the approach I currently have for <8Byte length and little endian is really fast, only 2 shifts + 3 operations (+-).

@Murmele
Copy link
Contributor Author

Murmele commented Jan 25, 2023

Good catch on the endianess! Not sure how i missed that or flipped it... sorry about the headache with that one. Thank you for the work on this one! Here are a few more comments on it.

Not sure completely sure what you meant about this comment since I didn't see it in the PR.

// Only little endian supported yet!
        // All signals must be little endian
        for (const auto& signal: m_signals) {
            if (signal.is_bigendian)
                return false;
        }

Not sure what is the best way to get the signals out

I think you did a good job with this interface to get them out.

Sorry I forgott to push. Now

@Murmele Murmele marked this pull request as ready for review January 28, 2023 19:18
@Murmele
Copy link
Contributor Author

Murmele commented Jan 28, 2023

@LinuxDevon fixed the conflict and the test. I have to check when I find the time to implement also bigendian....

@Murmele
Copy link
Contributor Author

Murmele commented Feb 8, 2023

@LinuxDevon I cleaned the branch up and added support for big endian as well

Reason:  otherwise it might search through the complete string to find a match
Files: dbc.cpp
Copy link
Owner

@LinuxDevon LinuxDevon left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this. Looks good! thank you for the work on this one! I'll try and get the format set and cleaned up here soon as we discussed in the other PR.

@LinuxDevon LinuxDevon merged commit 676da12 into LinuxDevon:master Feb 9, 2023
@Murmele
Copy link
Contributor Author

Murmele commented Feb 9, 2023

Hi @LinuxDevon thanks for merging. I am trying now to add ability to parse VAL_ symbols, but this is anymore that easy

@LinuxDevon
Copy link
Owner

@Murmele no worries. I haven't either and was my first attempt at one. Learned as I went. Let me know if you need any help or anything. I should hopefully have time in the up coming weeks to spend some time on this project

This was referenced Feb 9, 2023
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