Plan for feature integration

[atticusrussell]

What is the plan/strategy for adding new features?
PR #31 has a bunch of promising new features, and I was wondering what the process would be to get them merged.
I assume that adding ESP32 support, adding wifi transport, adding battery, should all be broken into separate PRs.

With the addition of CI, from now on we should know if the firmware will still build correctly for any particular PR, but I'm not sure this indicates if it will function as intended. Should we test that? Do we have any ideas for how unit tests might be done, or if they would even be desirable?

Would love to get your thoughts on this @grassjelly, and I would be willing to work on implementing whatever conclusion there is.

[grassjelly]

Would be great to have ESP32 (or Wifi enabled uCs) support, however that PR had a lot of intrusive changes especially on the core hardware libraries and the main firmware itself (which is understandable due to how the hardware interface works in ESP32 board as well as the different transport used. I think the least disruptive way is to:

• Create a new firmware folder that will house the new wifi-based firmware.
• Create new copies of existing sensor/hardware libraries that needs to be modified for ESP32 integration. This could be temporary and be merged to default libraries eventually, but in the meantime we'd want to find out which part of the libraries we need to poke.

Let me know what you think

[atticusrussell]

That's a good point. I honestly haven't looked super closely at the diff of that PR, so while those steps seem sane to me I think it would be good to get the opinion of @hippo5329 about how to go about merging features.

There seem to be a bunch of features added in that PR that would be less intrusive and would be of immense value to existing users on existing boards. Just to name a few of them:

• Magnetometer support
• Ultrasonic sensor support
• Battery voltage & current measurement
• Battery current measurement
• Reduced stopping distances
  I personally would love battery info for my robot, so wish to see this integrated into the current code.

It seems to me the PR should be broken up into PRs for individual features? So each of the bullet points above, as well as ESP32 support and pico-w support should each be separate PRs. Would be a bit of work, but I would love to see this repo get those features in a well-tested way.

I'm pretty interested in the idea of creating unit tests for this codebase, as I think each PR that adds features should be tested both to function and to not interfere with existing functionality. I also personally want to gain some experience with unit testing, and I think this repo is a good place to try it. It seems that PlatformIO has pretty good support for it.. Does it make sense to you if I create unit tests for the existing code?

[grassjelly]

I'm open to more PRs (ie. unit tests and new sensors). I do have a few concerns tho on the new features proposed:

This should be fine assuming it'll use the i2c interface.

• Magnetometer support

The Teensy boards might not have enough pins to support these sensors but I'm open to i2c based implementation).

• Ultrasonic sensor support
• Battery voltage & current measurement
• Battery current measurement

Not really sure what you mean by this

• Reduced stopping distances

[atticusrussell]

Whoops, didn't mean to list battery info twice as a feature.

What I meant by reduced stopping distance was @hippo5329 's addition of USE_SHORT_BRAKE, that from my very quick and cursory glance at some of the code, seems to attempt stopping more aggressively by locking the wheels by enabling both directional pins of the motor driver, instead of simply putting them in a neutral state with pwm of 0. Not totally sure, but also noticed a section of his PR branches README dedicated to it.

[grassjelly]

Understood. Honestly, I don't have the bandwidth at the moment to develop those features, but If you're okay to work on it, I'll be happy to review PRs,

[grassjelly]

hey @atticusrussell this might be a good start https://robofoundry.medium.com/running-linorobot2-hardware-based-on-micro-ros-on-esp32-wroom-32d-using-wifi-transport-d971026f3e08?source=social.tw

[hippo5329]

Dear all,

I am just back from traveling. I will follow up soon. Thanks.

1. About the "Reduced stopping distance" is using the "slow decay mode" to control the motors. This will give much better motor speed control at low speed and reduce the stopping distance of the robot to avoid collision just like that of driving a car. The configuration should become default. Please see the following link, https://learn.adafruit.com/improve-brushed-dc-motor-performance/current-decay-mode

2. Sorry for the confusion on my ESP32 PR. I had been a contributor/maintainer of Linux Kernel and u-boot almost 20 years ago. I am very familiar with the patches submitting and reviewing process. I had tried to keep my patches clean and ready to merge. But I did not get a single reply from my patches. I am glad they got some attention now. I will create a new branch and resubmit my patches in groups. Let's work together.

3. The linorobot is a great project. If you have limited time, please consider let some of us join the maintainers team.

Cheers,
Thomas

[atticusrussell]

Thomas, it's great to hear from you. Your work on the new features is very exciting! Would love to work together to get them merged.

Last week I began work on creating unit tests for the existing libraries. Despite the fact that these unit tests are far from complete or comprehensive, I will try to get a PR created that incorporates the created unit tests into CI.

My plan behind this is that any PR that adds functionality will also need unit tests written for those functions, and can be verified not to cause regressions from any of the existing code, in order to be merged.

Would love to hear if anyone agrees or has differing opinions - all feedback is welcome.

[hippo5329]

Atticus, Thanks for your efforts on unit testing. It is great to have tests code.

I have tried to limit the scope of each change with configuration and run building tests on all available targets. So that the regressions from existing code should be minimized.

BTW. In README.md, I have described the configuration to reduce the stopping distance is
USE_SHORT_BRAKE - Short brake for shorter stopping distance, only for generic_2 BT6612 and BTS7960 like motor drivers

Result with USE_SHORT_BRAKE.
MOTOR1 SPEED 0.46 m/s 4.13 rad/s STOP 0.014 m
MOTOR2 SPEED 0.49 m/s 4.36 rad/s STOP 0.024 m

Result without USE_SHORT_BRAKE.

MOTOR1 SPEED 0.46 m/s 4.10 rad/s STOP 0.135 m
MOTOR2 SPEED 0.49 m/s 4.40 rad/s STOP 0.171 m

You can see that the stopping distance is much longer without USE_SHORT_BRAKE. It can be dangerous in some case. The short brake also provides better speed control.

[atticusrussell]

@hippo5329, I wanted to let you know that I just created PR #65 that adds unit tests for some of the existing code. I hope the tests in the PR can provide an example of how to add unit tests to PlatformIO so that each PR proposing a new feature can be accompanied by unit tests. Please see the addition to the README in the PR if you have any confusion, and I would also be happy to elaborate if necessary.

@grassjelly I only created unit tests for a small bit of the existing code in the PR, but I aim to create more in follow-up PRs in the future as my schedule allows.

[hippo5329]

I submitted a pr for esp32 support onto rolling. It should be the same for other ROS2 distro. I am not familiar with the CI testing. Will the pr run through all the tests?