Protobuf integration with nanopb

[lmoesch]

For a project of mine I wrote a very crude (due to my limited experience) external lbuild-repository to integrate protocol buffers via nanopb into modm:

https://github.com/lmoesch/modm-nanopb

The reason to wrap it into a seperate repo, instead of direct modm-integration, is that this needs an additional python package installed (protobuf, suprise suprise).

Since I plan on refining the code for general public use, I wonder whats "best pratice" in this case, to provide it to all interested modm users.

[rleh]

Nice to see Protobuf/NanoPB used with modm!

The reason to wrap it into a seperate repo, instead of direct modm-integration, is that this needs an additional python package installed (protobuf, suprise suprise).

Sounds good!

Since I plan on refining the code for general public use, I wonder whats "best pratice" in this case, to provide it to all interested modm users.

I think the best thing to do is look at @salkinium|s repo https://github.com/modm-io/Invensense-eMD (if you haven't already) and adopt the structure.

Maybe @salkinium has more/better advice?

[salkinium]

1. It looks very self-contained, so we could just put it into ext/ as a git submodule via a modm-ext fork (that only pulls in the required sources of the latest release, see this script as an example) and expose it as a "normal" modm:nanopb module.
2. It would be better to implement the protoc call in the build system (nanopb comes with SCons integration!), instead of lbuild (which is fairly slow).
3. The additional python package would be optional, and we'd just put a note into the module doc that you need it in your path.

[lmoesch]

2. It would be better to implement the protoc call in the build system (nanopb comes with SCons integration!), instead of lbuild (which is fairly slow).

Wouldn't this mean, that the generated protocol headers/sources are just available after running scons build or do I miss the point? Because this would impede intellisense (or similar) capabilities.

[salkinium]

Yes, that's true. In that case would also add an explicit scons protoc target (or similiar) that builds just these files. Same for make protoc and whatever magic cmake requires 😒.

The nanopb build system integrations look pretty useful at first glance, perhaps we can copy them over and call them from our existing build systems?

Edit: I guess scons/make nanopb would be a better name? protoc is a little… vague.

[lmoesch]

The nanopb build system integrations look pretty useful at first glance, perhaps we can copy over and call them from our existing build systems?

Yeah I thought that too, I will try to reuse them and see how far I get with these :D

[rleh]

It would be better to implement the protoc call in the build system (nanopb comes with SCons integration!), instead of lbuild

Why? I consider the separation of code generation and compilation, as well as the build system independence to be very advantageous.

[lmoesch]

I tried to get a little bit more familiar with lbuild and scons. As far as I can tell, additional build targets that are running certain scripts instead of standard c/cpp compiling need to be specified via "Command" in the SContruct file, which would require to modify the SContruct generation within the modm scons module (correct me, if there is an easier solution).

Additionally we cannot use "nanopb" as build target command, since this is already a build target (via the static library).

Right now I tend to support the oppinion of @rleh since this is the much simpler solution with the only downside that the lbuild "build" have to be triggered everytime the .proto definition is changed.

[salkinium]

Ok, then let's do it this way. Even though I don't like to add more responsibilities to lbuild, it's better than not having nanobp at all and we can add build system support later.

I've created a modm-ext/partial-nanopb repo and added you as maintainer. Copy an update script and the GitHub Actions ci from another repo to create a minimal nanobp git submodule that you can add to modm/ext/nanobp/nanobp. Then place your nanobp lbuild repo's code into that.

[lmoesch]

I've created a modm-ext/partial-nanopb repo and added you as maintainer. Copy an update script and the GitHub Actions ci from another repo to create a minimal nanobp git submodule that you can add to modm/ext/nanobp/nanobp. Then place your nanobp lbuild repo's code into that.

Are you sure I'm added as maintainer, because it seems like I'm not allowed to push branches right now.

[salkinium]

I've resent your invite. I accidentally created a partial-nanobp repo, which I then had to rename to partial-nanopb, so maybe that got GitHub confused.

[salkinium]

There seems to be 0.3.x and 0.4.x, as well as nanopb-0.3.x and nanopb-0.4.x tags.
I think you may need to read all tags and filter to the largest one (use distutils.version.StrictVersion to compare), otherwise we might get some version thrashing 🤪.

[lmoesch]

distutils.version.StrictVersion

I checked the output from the github api call and it seems that the 0.x.x Tags are somehow only present in the ui, the api taglist contains only the nanopb-0.x.x tags.

[salkinium]

I renamed the repo again to nanopb-partial, changed the default branch to main and squashed all your changes into one commit to be in line with the remaining repos.

[salkinium]

Did you make progress with adding the nanopb module into modm/ext?

[lmoesch]

Did you make progress with adding the nanopb module into modm/ext?

I was pretty occupied by work the last weeks, so I couldnt find the time to progress with my private projects including the nanopb integration. I'll try to make the small PR for nanopb this week.

[lmoesch]

Small update:

I prepared everything so far for a pull-request to modm. Problem right now is that I cannot get protoc running with the current release tags of nanopv (cannot find correct python parser), so I guess their master (which is working) has some crucial bugfix.

I keep digging into the problem.

Update2:

Yeah, its a hotfix for Windows from 13.04.2021 which fixed the problem, hopefully this gets into a new releasetag soon.

[salkinium]

Do you want to open a Draft PR regardless? I would like to see the nanopb failure integrated into the (Windows) CI for future refactorings.

[lmoesch]

Sorry for the long wait - the PR is now online (#657)