Conversation
|
Actually I never wrote a parser, so I don't know if it is performant or not ... |
Reason, in a previous commit a second message was added
There was a problem hiding this comment.
Letf a few comments @Murmele. I apologize for the delay. I have been pretty busy and not motivated to maintain this repo lately. I appreciate the effort here!
I am trying to get some automation in here and refactor the repo some. I am not too happy with the current structure since it was the first stab at learning on some of these ideas.
include/libdbc/message.hpp
Outdated
|
|
||
| void appendSignal(const Signal& signal); | ||
| const std::vector<Signal> signals() const; | ||
| const std::vector<Signal>& getSignals() const; |
There was a problem hiding this comment.
I don't like the reference here since that ties it to the lifetime of the object now.
There was a problem hiding this comment.
Good point, I can change this
| std::vector<libdbc::Signal::SignalValueDescriptions> vd; | ||
| }; | ||
|
|
||
| bool parseVal(const std::string& str, VALObject& obj) { |
There was a problem hiding this comment.
This should be a member on the class of signal not external imo. External classes shouldn't know of these details just that they get the members after. Also not a huge fan of how long this function is. Perhaps split this up into some sub functions?
I am trying to follow what is going on and having a bit of a hard time. I will take another look again when I get a chance here.
There was a problem hiding this comment.
i don't think so, because this is related parsing the val object. It does the same thing like the regular expression, but in a different way because of the unknown numbers of value labels available. Therefore regex is not possible here
| std::vector<VALObject> sv; | ||
|
|
||
| VALObject obj; |
There was a problem hiding this comment.
This come back to the comment before that the message shouldn't be parsing this. Maybe the val should be found first then passed to the signal to find the ones it needs? I guess I need to think about this some and maybe the function being a member on the signal isn't the right choice.
There was a problem hiding this comment.
The idea was first parsing the dbc file and then assigning the val object to the signal
| Message msg(id, name, size, node); | ||
|
|
||
| messages.push_back(msg); | ||
| continue; |
There was a problem hiding this comment.
If the regex matches a message is found and therefore it is not needed to check the others, because they will never match. So I can go directly to the next line
| for (const auto& obj : sv) { | ||
| for (auto& msg : messages) { | ||
| if (msg.id() == obj.can_id) { | ||
| msg.addValueDescription(obj.signal_name, obj.vd); |
There was a problem hiding this comment.
I see now that this is done through a function on the message. I see why it is done this way now I think.
|
@LinuxDevon thank you for thre review! I will work on your comments. Currently in Labplot we are using my patched version, but if we get my stuff in I can switch to upstream. I know it is a big work to maintain a project if a lot will change. On my side I don't have much more work on this library, so I think those are the last MR from my side. |
There was a problem hiding this comment.
@Murmele sorry for the delay. Not sure if all the points were addressed but going to approve this and come back later to clean up. I will work on a release of this. The interface might change a bit in the future.
2 participants
Reason: It can be used to give the signal states for its values