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_DroneCAN: DNAServer: remove preferred allocation support #28100

Merged

Conversation

tpwrules
Copy link
Contributor

@tpwrules tpwrules commented Sep 14, 2024

Nothing is known to support it so it can't be tested. Removing it saves flash and reduces complexity.

I would prefer to add support for it to AP_Periph so it can be tested but tridge said he would prefer to just remove support. Saves 168 bytes of flash on Cube Orange. Would consume 32 bytes to add support to AP_Periph.

Tested on the bench that allocation of new and existing nodes works properly still.

Copy link
Member

@bugobliterator bugobliterator left a comment

Choose a reason for hiding this comment

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

Agreed. Currently if user wants to force node ids, its best to statically assign on the device, and let the dnaserver register it statically. We should probably verify that our prearm works if it sees multiple devices with same node ids.

@tpwrules
Copy link
Contributor Author

I just checked that on the bench and it does still work with one ID manually assigned and the other automatically, or both manually. I think there are some quirks though and I plan to go over that functionality with a fine toothed comb soon.

Nothing is known to support it so it can't be tested. Removing it saves
flash and reduces complexity.
@tridge tridge force-pushed the pr/dna-remove-preferred-alloc branch from 727f638 to 0c1cf1f Compare September 17, 2024 00:42
@peterbarker peterbarker merged commit 20d04fa into ArduPilot:master Sep 17, 2024
95 checks passed
@tpwrules tpwrules deleted the pr/dna-remove-preferred-alloc branch September 17, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants