Skip to content

Ptt/cleanup#454

Merged
avtoku merged 18 commits intomainfrom
ptt/cleanup
Jul 22, 2025
Merged

Ptt/cleanup#454
avtoku merged 18 commits intomainfrom
ptt/cleanup

Conversation

@avtoku
Copy link
Contributor

@avtoku avtoku commented Jun 19, 2025

In base rosflight_firmware:
+Moved GNSS data type conversion from mavlink into board
+Removed ublox gnss fix type
+Removed unused variables in rosflight_structs.h associated with unused ublox packets.
+Updated parameters_test.cpp to match param.cpp
+Removed cases in void Mixer::param_change_callback(uint16_t param_id) that don't require action and were causing undesired recursion.
+Removed unnecessary ublox M9 protocol, defaulting to legacy M8 protocol.

In boards/varmint_h7:
+Created DoubleBuffer interface
+Replaced PacketFifo with DoubleBuffer for sensors since it's more efficient and we only ever used the most recent value read for sensors. Also removed associated, and now unused, #defines in BoardConfig.h's
+Added type conversion for GNSS data to match new rosflight_structs.h
+Removed .clang-format from boards/varmint_h7 (did not run formatter to avoid spurious changes during review)
+Pass header structure into misc_header().
+Added MiscRotatable to misc.h. Used to align sensor with each other and the board axes.
+Removed external SPI in Pixracer Pro version. Replace with 3 GPIO probe outputs and One EXTI input for GPS PPS
+Enlarged heap and stack to avoid potential overflow noting that there is a lot of free RAM to do this with.
+Used known group delay of IMU's to set timestamp to time of validity vs. data ready time. This is consistent with what was done with the GPS timestamp.

  • increase rx buffer size to 8k for telem and vcp circular fifo buffers. Also move them to Domain 1 AXI SRAM from DTCRAM

Cleanup:
+Replaced build_varmint.sh with unit_test.sh
+Added CMakeFiles to .gitignore
+Added CMakeCache.txt to .gitignore
+Moved read_complete in rosflight_structs.h structures into the header structure as "complete"
+Removed some unused and commented-out code

Todo:

Run formatter

@avtoku avtoku requested review from JMoore5353 and iandareid June 19, 2025 15:31
void Mixer::init() { init_mixing(); }
void Mixer::init()
{
load_primary_mixer_values();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With these two function calls here, a call to mixer::init will load the mixer values from the parameters stored, and then will call init_mixing, which will reload the same values to the mixing matrix. This means we'll load the mixer values twice for a given mixer configuration on startup.

The only case where it would be different is if PRIMARY_MIXER is set to an invalid number, in which case the primary_mixer_ variable in init_mixing would not be set, and would thus be holding on to a mixer that is not currently selected.

Is that correct? Is there a reason we should be including these two calls here in addition to the calls in init_mixing?

Copy link
Contributor Author

@avtoku avtoku Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured that at the end of ROSflight::init() the values should not be un-initialized. We could also just set them all to zeros. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good call... They shouldn't be uninitialized by the end of ROSflight::init(). Loading them with zeros is a good idea. That also has an additional layer of safety (outputs will be zero) if something happens with the mixer and it mixes the outputs even when PRIMARY_MIXER is set to an invalid mixer.

@JMoore5353 JMoore5353 self-requested a review July 18, 2025 15:19
void Mixer::init() { init_mixing(); }
void Mixer::init()
{
load_primary_mixer_values();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good call... They shouldn't be uninitialized by the end of ROSflight::init(). Loading them with zeros is a good idea. That also has an additional layer of safety (outputs will be zero) if something happens with the mixer and it mixes the outputs even when PRIMARY_MIXER is set to an invalid mixer.

@iandareid
Copy link
Contributor

I discovered a bug. Running param_load_from_file does not load all of the parameters. I have run it about 15 times and sometimes it updates parameters and sometimes it does not.

It looks like only a few parameters get loaded on each call. Sometimes it does not load any parameters. I can run param_write with no issues. Any ideas @avtoku @JMoore5353?

Copy link
Contributor

@iandareid iandareid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are bugs when loading parameters from a file.

@avtoku
Copy link
Contributor Author

avtoku commented Jul 21, 2025

Maybe a buffer full somewhere and no retry mechanism (the firmware echos the parameter back).
The parameter data exceeds the input buffer size; if using USB/VCP the data is likely getting truncated.
Was this over USB/VCP or UART/TELEM? What platform was this on - Varmint or Pixracer Pro?

@iandareid
Copy link
Contributor

This was on the Varmint. However, your latest commit has fixed the issue.

@avtoku
Copy link
Contributor Author

avtoku commented Jul 22, 2025

8k buffers for the incoming packets is kind of excessive, though we do have a lot of RAM. At some point we should slow down the parameter packet rate from the io node and implement some sort of handshaking and/or retry logic. The firmware already echos back a successful parameter read, so that can be used for verificaiton.

@avtoku avtoku merged commit 8662ea3 into main Jul 22, 2025
2 checks passed
@avtoku avtoku deleted the ptt/cleanup branch July 22, 2025 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants