Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AP_FrSky_Telem: Don't retain the object if no ports were found #11612

Merged
merged 3 commits into from Jun 26, 2019

Conversation

WickedShell
Copy link
Contributor

@WickedShell WickedShell commented Jun 18, 2019

This is something I've wanted to mess with for awhile now. The basic idea is that if we don't have a serial port for it, we can't possibly have a useful instance of the FrSky telem, so skipping updating it saves time and memory.

The overall RAM savings is 376 bytes, it's split between two commits:

  • First just to delete the instance if we didn't find a port, this only saved 96 bytes
  • Second we were storing the statustext_queue as a static for some reason, which was another 280 bytes, and was generally a bad idea (particularly if we ever wanted to run 2 instances at once).

I've tested this on a Cube (running Plane is where the numbers came from) without any FrSky setup, I have the hardware in a box to test with FrSky, but I've never actually gotten to setting it up, so if someone can help with validating that it would be appreciated.

Are there any objections to this path of removing stuff on startup that we don't need? I'd like to extend this to also cover Devo, but only did FrSky as proof of concept. EDIT: Devo is maybe not worth the effort, it holds a uint32_t and pointer... For the 8 bytes of RAM I'd spend more in flash...

@@ -2279,7 +2280,13 @@ void GCS::setup_uarts(AP_SerialManager &serial_manager)
chan(i).setup_uart(serial_manager, AP_SerialManager::SerialProtocol_MAVLink, i);
}

frsky.init();
if (frsky == nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be some sort of assert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the thought. It looks like we only ever call it once at the moment, but nothing strictly enforced that, and I didn't chase if setup_uart is actually fine a second time. But if you think it should be a sitl panic I can make it do that.

Copy link
Member

@OXINARF OXINARF left a comment

Choose a reason for hiding this comment

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

LGTM

@OXINARF OXINARF added Library and removed MAVLink labels Jun 18, 2019
@@ -33,12 +33,15 @@

extern const AP_HAL::HAL& hal;

ObjectArray<mavlink_statustext_t> AP_Frsky_Telem::_statustext_queue(FRSKY_TELEM_PAYLOAD_STATUS_CAPACITY);
AP_Frsky_Telem::AP_Frsky_Telem(void) :
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder... could we eliminate this constructor altogether by using brace-initialisation of _statustext_queue within the header file?

Happy to merge this and do that as a future PR...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I messed around a bit and got the syntax wrong, so I moved it out cause I knew the syntax for that :) Will come back to trying with braces.

@tridge
Copy link
Contributor

tridge commented Jun 26, 2019

tested on KakuteF4, works well

@tridge tridge merged commit 67898d7 into ArduPilot:master Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants