Closed
Conversation
We need more connection leve diagnostics in our logs but turning on DEBUG for roscpp_internal generates a flood of information. We only need visibility into when tcpros connections are created/dropped for publishers and subscribers. The rospy nodes currently do this but the roscpp ones do not. A DEBUG macro for logging messages to the named logger roscpp_internal.connections has been added. This will allow it to inherit the roscpp_internal log level or be turned on separately to just get connection-related messages.
Add logger roscpp_internal.connections.
* Implemented rate-control-topic and rate-control-max-delay. * Made rate control comparisons compare to time publisher rather than wall time. * Added a check and a warning if the header is not the first in the message, and removed an unnecessary import. * Some quick formatting changes to simplify the final pull reuqest. * Why is there little whitespace changes. * Tabs to spaces, and other whitespace fixes. * This is the last whitespace fix I swear. * Removed unused function parameters. * Fixed a bug preventing code from building after commit ecfb85f. * Aligned print messages for rosbag PAUSED/DELAYED/RUNNING
* add test code for multibyte string * merge Python 2 and 3 code paths
… confusing. For example, [this warning msg](https://travis-ci.org/wg-perception/people/jobs/202019288#L3737) (in this [PR](wg-perception/people#49)): ``` * WARN: unrecognized 'group' tag in <node> tag. Node xml is <test name="$(arg testnode_name)" pkg="rostest" test-name="hztest_test" type="hztest"> <group unless="$(arg expected_success)"> <node args="-r 0.5 $(find face_detector)/test/face_detector_noface_test_diamondback.bag" name="play" pkg="rosbag" type="play"/> <param name="hz" value="0.0"/> </group> ``` This is confusing because the tag in question is actually `<test>`, not `<node>`. Although the warning message refers to the exact <test> tag, I didn't try to read it because I was told the issue is with <node>, not <test>. With this PR the message becomes a bit verbose but hope the readers get better idea.
|
Not quite. Please target the kinetic devel branch upstream. |
fixes CORE-6641 Consider the following scenario for a message G with the following versions: G0 -- rule.bmr --> G1 -- auto-generated rule --> G3 where G3 is the CURRENT msg, and G2 is the BAG message that needs migration. We need to check for a migration rule like this first: G2 --> G3 instead of the backwards ones that were generated before this bug fix: G2 --> G1 Note that the 'lazy' check to valid the rules in the make_update_rule method makes that rule pass initially, but later it fails because there's no rule for the sub fields.
refs CORE-6641 Checking the sub rules are also valid we allow to check for different positions in the chain for the BAG message we need to migrate, because we don't really know where it should go, and the 'lazy' check for valid in the make_update_rule method makes the suggested rules pass almost always, so we can easily get a rule in the wrong place that later would fail because there are no rules for the sub fields.
97fdf48 to
0c64db0
Compare
Member
Author
|
OK, rebased on top of |
This file contains hidden or 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
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.
Description of the buggy corner case not handled on the rosbag migration tool
Consider the following scenario for a message G with the following versions:
where
G3is the current msg, andG2is the bag message that needs migration.We need to check for a migration rule like this first:
instead of the backwards ones that were generated before this bug fix:
Note that the lazy check to mark the rules as valid in the
make_update_rule(see https://github.com/clearpathrobotics/ros_comm/blob/kinetic-devel-cpr/tools/rosbag/src/rosbag/migration.py#L1123) method makes that rule pass initially, but later it fails because there's no rule for the sub fields.This has been adapted from the commit ec8ec37 description.
Improvement to the validation check for the rules
The other commit 97fdf48 simply allows to improve the lazy check in
make_update_ruleand therefore allows to find a better placement/position for the message to be migrated inside of the chain rules. This helps to avoid marking rules as valid, that later would fail because there's no rule for the sub fields. The more precise check that fails later happens at the sub fields level, and there's already a comment on the code that suggest that the way this is handled should be improved (see https://github.com/clearpathrobotics/ros_comm/blob/kinetic-devel-cpr/tools/rosbag/src/rosbag/migration.py#L307).FYI @servos @guillaumeautran
FYI @mikepurvis (Can you confirm I'm correctly targeting
kinetic-edgehere?)