BcbBattery #749
Merged
BcbBattery #749
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
randaz81
force-pushed
the
bcbBattery2
branch
3 times, most recently
from
76a654a
to
35a711a
Compare
S-Dafarra
reviewed
May 21, 2021
|
I tried on Click to expandApparently I was not able to close it cleanly, and I had to kill it. Just for reference, this is the output with the upstream version: Click to expand |
|
I apologize for the time you already spent on the review. I pushed an update with some MAJOR changes. Especially the logic is (again) completely different. To improve the readability, I deleted some of your comments which I am 100% sure do not apply anymore. |
I think |
|
Some considerations for improving the transmission protocol (some changes to firmware are required @MrAndrea )
unsigned char calculateLRC(unsigned char *buf, unsigned int n)
{
unsigned char checksum = 0;
while(n>0)
{
checksum += *buf++;
n--;
}
return ((~checksum) + 1);
} |
pattacini
reviewed
May 23, 2021
There was a problem hiding this comment.
Thanks @randaz81 for the insights!
A few comments:
- Please, don't delete comments, but just mark them as resolved explaining why.
- It's quite important to understand if we depend on
yarp@3.4.4oryarp@mastersince we're about to yield a new distro. If we depend onyarp@master, I'm afraid we have to postpone the PR. On the other hand,yarp@3.4.4is totally ok. cc @Nicogene @traversaro. Anyway,yarp@3.4.4has been delivered a few days ago so it's quite likely that it's the version required here. - Can you clarify if the module shuts down now smoothly? I think so but it wasn't 100% clear from your latest comments.
S-Dafarra
suggested changes
May 24, 2021
| yDebug("BcbBattery::run() serial_buffer is: (hex) %s, (dec) %s", hexBuffer, decBuffer); | ||
| printf("internal buffer:"); | ||
| for (size_t i = 0; i < recb; i++) | ||
| printf("%02X ", (unsigned int)(tmp_buff[i] & 0xFF)); |
There was a problem hiding this comment.
Please, keep also the decimal printout, as it was before. This is useful to debug if the incoming values make sense without having to convert them from hexadecimal to decimal.
There was a problem hiding this comment.
well, the regex cannot fail, so a correct representation of the packet is always given in the subsequent hex representation of the packet (line ~173 of the latest commit) . In that debug print, decimal representation is no more useful since they match 1:1 with the values you can read from the interface...
There was a problem hiding this comment.
Still, we could have problems at the sender level. That has been useful on few occasions in the past.
| //parse battery data | ||
| std::smatch match; | ||
| string stringbuf((char*)tmp_buff, recb); | ||
| bool b = std::regex_search(stringbuf, match, r_exp); |
There was a problem hiding this comment.
Is regex_search safe to be used given that the first char of the input string is supposed to be a \0 (given the communication protocol the BCB uses)?
I did not manage to find any indication about this, but in https://en.cppreference.com/w/cpp/regex/regex_search, one of the documented usages involves a null-terminated string. If the input string is converted into a C string, then it may cause false negatives I suppose. I don't know if this can be implementation-dependent.
There was a problem hiding this comment.
well, I tested it and I haven't noticed any problem... But that was my machine, I really hope that that this is not implementation dependent!
Thanks for the insights. Could we solve this by simply adding a |
S-Dafarra
reviewed
May 24, 2021
@S-Dafarra @pattacini the correct tag is |
pattacini
approved these changes
May 24, 2021
There was a problem hiding this comment.
@S-Dafarra @pattacini the correct tag is yarp@3.4.5, sorry for the mistake
Thanks @randaz81 👍🏻
Just awaiting the final confirmation from @Nicogene and/or @traversaro on this.
CI is fine, so as long as @S-Dafarra is ok with the current status, I'll be happy to squash'n'merge.
Ok YARP v3.4.5 for me. |
Nicogene
reviewed
May 24, 2021
There was a problem hiding this comment.
Preface: I have no insight in this device.
These changes shouldn't require that find_package(YARP 3.4.5)? In that case could be a problem for the packets creations, since we have not the yarp 3.4.5 deb packets ready, and the GA that build the icub packets install yarp from packets.
We can put this requirement in the main
My point was right to check if we can officially incorporate |
Just for extra clarity: yarp.3.4.5 is not required to compile this device. However, if you do not have yarp.3.4.5, the device will behave in erratic way at runtime, since there was a bug in the wrapper, which was fixed indeed in 3.4.5. |
For this, I think we are out of time because we need to make the deb packets of yarp and upload them at least on Github. We can put the tag in the yml of the superbuild, but we cannot put the requirement in the CMakelists, unless someone manage to do the packets before 31/05
So in the actual state on |
|
I can see a couple of solutions then:
How do you see that? |
|
I just tried it on the robot again, but there was an exception thrown by |
Wow, this is pretty weird. I cannot get this error. According to what I was able to find on google, the error says that the regex expression definition (line 59, .h file) is incomplete, but on my setup (Ubuntu 18.04.2, gcc 7.4.0) it works fine. PS: Tested successfully also on VS2019 |
I tested it on the |
|
Can you do a test also on a different machine? In any case, tomorrow I'll be in lab and we can check icub head setup together. |
I am pretty interested to have it working on the |
|
We're running a bit late, in agreement w/ @Nicogene we propose the following plan:
|
S-Dafarra
approved these changes
May 26, 2021
There was a problem hiding this comment.
It works pretty well and closes smoothly now!
Here the output on the robot head
[DEBUG] Reading file /usr/local/src/robot/robotology-superbuild/src/robots-configuration/iCubGenova04/battery/icub_battery.xml
[INFO] |yarp.os.Port| Port /log/icub-head/yarprobotinterface/7269 active at tcp://10.0.0.2:10013/
[DEBUG] yarprobotinterface: using xml parser for DTD v3.x
[DEBUG] Reading file /usr/local/src/robot/robotology-superbuild/src/robots-configuration/iCubGenova04/battery/icub_battery.xml
[DEBUG] Preprocessor complete in: 0.00011611 s
[INFO] |yarp.os.Port| Port /battery/yarprobotinterface active at tcp://10.0.0.2:10003/
[INFO] startup phase starting...
[INFO] |yarp.devices.BatteryWrapper| Using period: 0.1 s
[INFO] |yarp.os.Port| Port /icub/battery/data:o active at tcp://10.0.0.2:10004/
[INFO] |yarp.os.Port| Port /icub/battery/rpc:i active at tcp://10.0.0.2:10005/
[INFO] |yarp.dev.PolyDriver| Created wrapper <batteryWrapper>. See C++ class BatteryWrapper for documentation.
[DEBUG] (GENERAL (name icub_battery) (thread_period 100) (screen 1) (verbose 1) (debug 0) (silence_sync_warnings 1)) (SERIAL_PORT (verbose 1) (comport "/dev/rfcomm0") (baudrate 115200) (xonlim 0) (xofflim 0) (readmincharacters 0) (readtimeoutmsec 5) (parityenb 0) (paritymode none) (ctsenb 0) (rtsenb 0) (xinenb 0) (xoutenb 0) (modem 0) (rcvenb 0) (dsrenb 0) (dtrdisable 0) (databits 8) (stopbits 1) (line_terminator_char1 10) (line_terminator_char2 10)) (device bcbBattery) (robotName battery)
[INFO] |yarp.device.serialport| Starting Serial Port in /dev/rfcomm0
[INFO] |yarp.dev.PolyDriver| Created device <serialport>. See C++ class SerialDeviceDriver for documentation.
[INFO] BcbBattery starting transmission
[DEBUG] |yarp.device.serialport| Sending string: ����`
[INFO] BcbBattery started successfully
[DEBUG] Internal buffer:
[INFO] |yarp.dev.PolyDriver| Created device <bcbBattery>. See C++ class BcbBattery for documentation.
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:57 2021
[INFO] Entering action level 5 of phase startup
[INFO] icubBattery is not an IWrapper. Trying IMultipleWrapper
[INFO] All actions for action level 5 of startup phase started. Waiting for unfinished actions.
[INFO] All actions for action level 5 of startup phase finished.
[INFO] startup phase finished.
[INFO] run phase starting...
[DEBUG] yarprobotinterface running happily
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:57 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:57 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:58 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:58 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:58 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:58 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:58 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:58 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:58 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:58 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:58 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:58 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:59 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:59 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:59 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:59 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:59 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:59 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:59 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:59 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:59 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:29:59 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:30:00 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 0.0V +0.0A, charge: 0.0%, time: Wed May 26 17:30:00 2021
[DEBUG] Internal buffer: 00 9D 9E 02 D0 00 53 80 0D 0A
[DEBUG] Found: <00> (9D 9E) (02 D0) (00 53) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 83.0%, time: Wed May 26 17:30:00 2021
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:00 2021
[DEBUG] Internal buffer: 00 9D 9E 02 D0 00 53 80 0D 0A
[DEBUG] Found: <00> (9D 9E) (02 D0) (00 53) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 83.0%, time: Wed May 26 17:30:00 2021
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:00 2021
[DEBUG] Internal buffer: 00 9D 9E 02 D0 00 53 80 0D 0A
[DEBUG] Found: <00> (9D 9E) (02 D0) (00 53) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 83.0%, time: Wed May 26 17:30:00 2021
[DEBUG] Internal buffer: 00 9D D0
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 83.0%, time: Wed May 26 17:30:00 2021
[DEBUG] Internal buffer: 02 D0 00 54 80 0D 0A
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 83.0%, time: Wed May 26 17:30:00 2021
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:00 2021
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:01 2021
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:01 2021
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:01 2021
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:01 2021
[DEBUG] Internal buffer: 00 9D D0 02 A8 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 A8) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:01 2021
[DEBUG] Internal buffer: 00 9D 9E 02 D0 00 53 80 0D 0A
[DEBUG] Found: <00> (9D 9E) (02 D0) (00 53) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 83.0%, time: Wed May 26 17:30:01 2021
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:01 2021
[DEBUG] Internal buffer: 00 9D 9E
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:01 2021
[DEBUG] Internal buffer:
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:01 2021
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:01 2021
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:02 2021
[DEBUG] Internal buffer: 00 9D 9E 02 D0 00 53 80 0D 0A
[DEBUG] Found: <00> (9D 9E) (02 D0) (00 53) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 83.0%, time: Wed May 26 17:30:02 2021
[DEBUG] Internal buffer: 00 9D D0 02 A8 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 A8) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:02 2021
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:02 2021
[DEBUG] Internal buffer: 00 9D 9E 02 D0 00 53 80 0D 0A
[DEBUG] Found: <00> (9D 9E) (02 D0) (00 53) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 83.0%, time: Wed May 26 17:30:02 2021
^C[INFO] |yarp.os.RFModule| [try 1 of 3] Trying to shut down.
[WARNING] Interrupt # 1 # received.
[INFO] Interrupt received. Stopping all running threads.
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:02 2021
[DEBUG] Internal buffer: 00 9D 9E 02 A8 00 53 80 0D 0A
[DEBUG] Found: <00> (9D 9E) (02 A8) (00 53) (80) <0D 0A>
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 83.0%, time: Wed May 26 17:30:02 2021
[INFO] |yarp.os.RFModule| RFModule closing.
[INFO] interrupt1 phase starting...
[INFO] interrupt1 phase finished.
[INFO] shutdown phase starting...
[INFO] Entering action level 5 of phase shutdown
[DEBUG] Internal buffer: 00 9D D0 02 D0 00 54 80 0D 0A
[INFO] All actions for action level 5 of shutdown phase started. Waiting for unfinished actions.
[DEBUG] Found: <00> (9D D0) (02 D0) (00 54) (80) <0D 0A>
[INFO] All actions for action level 5 of shutdown phase finished.
[DEBUG] BcbBattery::run() log_buffer is: 40.4V +0.7A, charge: 84.0%, time: Wed May 26 17:30:02 2021
[DEBUG] |yarp.device.serialport| Sending string:
[INFO] shutdown phase finished.
[INFO] |yarp.os.RFModule| RFModule finished.
Cool then 🎉 |
That was the message I put when I approved 😁 |
|
PR merged ✔ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some changes to: #727
Main changes and explanation:
I did some experiments and I noticed that the previous implementation was too much dependent on the thread period: a wrong value could cause severe misbehavior.
So: this is the logic of the new version:
On every run() cycle I copy all the content of the serial buffer to a local buffer and I search for the last (most recent) valid packet occurrence. This solution works as long as the thread period is at least twice the size of the duration of a packet (but I think that keeping it large, i.e. 500-1000ms is still a good idea)