Fix #2035: Initialize previous_publish_timestamp_ in on_configure

[Bharath4444]

Description

This PR addresses issue #2035 by moving the initialization of previous_publish_timestamp_ into the on_configure() lifecycle method.

Previously, this variable was initialized inside a catch block within the update loop, effectively using exception handling for normal control flow. Moving it to on_configure aligns it with how previous_update_timestamp_ is handled and ensures the timestamp is valid before the update loop begins.

Related Issues

Closes #2035

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings

[Amronos]

I think this variable can/should be removed now.

[saikishor]

I guess it is still needed to be able to control the frequency of publishing the message

[saikishor]

You are right, we can combine the logic and we can remove it

[saikishor]

Combine the logic and remove should_publish variable

[christophfroehlich]

This just removes the logic introduced with #585

Are we sure that this is not needed anymore?

[Amronos]

Suggested change

	previous_publish_timestamp_ += publish_period_;
	previous_publish_timestamp_ = time;

@christophfroehlich I think this should fix #585 and also be better overall.

[christophfroehlich]

no because the comparison above fails if the time source changed.

[Amronos]

Suggested change

	if (previous_publish_timestamp_ + publish_period_ <= time)
	if (previous_publish_timestamp_ + publish_period_ > time)

I was misunderstanding the logic here. I think with this change and the earlier one everything should work. This should cause this if statement to pass as true the first time in simulation and from there on work correctly.

@christophfroehlich

[christophfroehlich]

still: This comparison will throw an exception if the timesource changes

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

[mergify]

This pull request is in conflict. Could you fix it @Bharath4444?

[christophfroehlich]

@Bharath4444 could you fix the remaining threads please?

[christophfroehlich]

Please don't do force pushes if a PR is already under review. Have you changed anything, have you considered the remarks in the threads?

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.85%. Comparing base (b3e8a30) to head (04e7542).
⚠️ Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
...iff_drive_controller/src/diff_drive_controller.cpp	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2084      +/-   ##
==========================================
- Coverage   84.85%   84.85%   -0.01%     
==========================================
  Files         153      153              
  Lines       15016    15011       -5     
  Branches     1298     1297       -1     
==========================================
- Hits        12742    12737       -5     
  Misses       1800     1800              
  Partials      474      474              

Flag	Coverage Δ
unittests	84.85% <66.66%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...iff_drive_controller/src/diff_drive_controller.cpp	80.92% <66.66%> (-0.29%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[RicardoMan]

I read through the diff and the review comments. It seems the main issue is that removing the try-catch leaves no handling for when the clock source changes at runtime. The comparison on line 241 would still throw in that case.

One idea: check if the clock type changed before doing the arithmetic, using get_clock_type() from the rclcpp::Clock API (https://docs.ros.org/en/humble/p/rclcpp/generated/classrclcpp_1_1Clock.html). If the types don't match, reinitialize previous_publish_timestamp_ with the current time. This avoids using exceptions for normal control flow while still handling the timesource change.

[christophfroehlich]

Thanks for your work and comments, but I think we will just remove this functionality with #2245