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

docs: Add guide for creating your own platform #970

Merged
merged 1 commit into from
Feb 25, 2022

Conversation

jonasdn
Copy link
Contributor

@jonasdn jonasdn commented Feb 25, 2022

No description provided.

Copy link
Contributor

@krichardsson krichardsson left a comment

Choose a reason for hiding this comment

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

Wow that was fast!

It would also be possible to see this as a variation of a platform and only replace the default values, that is only this part.

#pragma once
#define __INCLUDED_FROM_PLATFORM_DEFAULTS__
#ifdef CONFIG_PLATFORM_CF2
    #include "platform_defaults_cf2.h"
#endif
#ifdef CONFIG_PLATFORM_BOLT
   // Not exactly sure how to do this but use platform_defaults_bolt.h if RINCEWIND is not selected
    #ifdef CONFIG_PLATFORM_BOLT_RINCEWIND
        #include "platform_rincewind.h"
   #else
        #include "platform_defaults_bolt.h"
    #endif
#endif
#ifdef CONFIG_PLATFORM_TAG
    #include "platform_defaults_tag.h"
#endif

It would reduce the complexity and also limit the possibility of messing up settings based on the hardware such as IMU type and motorMap. I'm not sure there is a use case where the hardware conf needs to be changed?

@jonasdn
Copy link
Contributor Author

jonasdn commented Feb 25, 2022

Wow that was fast!

It would also be possible to see this as a variation of a platform and only replace the default values, that is only this part.

#pragma once
#define __INCLUDED_FROM_PLATFORM_DEFAULTS__
#ifdef CONFIG_PLATFORM_CF2
    #include "platform_defaults_cf2.h"
#endif
#ifdef CONFIG_PLATFORM_BOLT
   // Not exactly sure how to do this but use platform_defaults_bolt.h if RINCEWIND is not selected
    #ifdef CONFIG_PLATFORM_BOLT_RINCEWIND
        #include "platform_rincewind.h"
   #else
        #include "platform_defaults_bolt.h"
    #endif
#endif
#ifdef CONFIG_PLATFORM_TAG
    #include "platform_defaults_tag.h"
#endif

It would reduce the complexity and also limit the possibility of messing up settings based on the hardware such as IMU type and motorMap. I'm not sure there is a use case where the hardware conf needs to be changed?

It is not possible with how we do it right now, since PLATFORM_BOLT and PLATFORM_RINCEWIND can never both be true when they are in a choice in Kconfig.

@jonasdn
Copy link
Contributor Author

jonasdn commented Feb 25, 2022

Wow that was fast!
It would also be possible to see this as a variation of a platform and only replace the default values, that is only this part.

#pragma once
#define __INCLUDED_FROM_PLATFORM_DEFAULTS__
#ifdef CONFIG_PLATFORM_CF2
    #include "platform_defaults_cf2.h"
#endif
#ifdef CONFIG_PLATFORM_BOLT
   // Not exactly sure how to do this but use platform_defaults_bolt.h if RINCEWIND is not selected
    #ifdef CONFIG_PLATFORM_BOLT_RINCEWIND
        #include "platform_rincewind.h"
   #else
        #include "platform_defaults_bolt.h"
    #endif
#endif
#ifdef CONFIG_PLATFORM_TAG
    #include "platform_defaults_tag.h"
#endif

It would reduce the complexity and also limit the possibility of messing up settings based on the hardware such as IMU type and motorMap. I'm not sure there is a use case where the hardware conf needs to be changed?

It is not possible with how we do it right now, since PLATFORM_BOLT and PLATFORM_RINCEWIND can never both be true when they are in a choice in Kconfig.

Instead maybe:

#pragma once

#define __INCLUDED_FROM_PLATFORM_DEFAULTS__

#ifdef CONFIG_PLATFORM_CF2
    #include "platform_defaults_cf2.h"
#endif
#ifdef CONFIG_PLATFORM_BOLT
    #include "platform_defaults_bolt.h"
#endif
#ifdef CONFIG_PLATFORM_TAG
    #include "platform_defaults_tag.h"
#endif
#ifdef CONFIG_RINCEWIND
    #include "platform_bolt.h"
    #include "platform_rincewind.h"
#endif

To first include bolt and then rincewind to override or add to what bolt has.

@krichardsson
Copy link
Contributor

Looks like a better solution to me :-)

@jonasdn
Copy link
Contributor Author

jonasdn commented Feb 25, 2022

So, @ntamas the idea we have that is for changing default values of parameters, you add your own platform. For instance for stuff like the critical voltage you added. That would be better as defines in a platform file.

So if you create for instance a collmot_defconfig with PLATFORM_COLLMOT we would be able to merge that and you can build by going make collmot_defconfig.

@jonasdn jonasdn force-pushed the jonasdn/doc-new-platform branch from 625f1d4 to 951a280 Compare February 25, 2022 10:18
@jonasdn jonasdn force-pushed the jonasdn/doc-new-platform branch from 951a280 to e3602e6 Compare February 25, 2022 10:19
Copy link
Member

@tobbeanton tobbeanton left a comment

Choose a reason for hiding this comment

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

Looks really good!

@jonasdn jonasdn merged commit f2d602e into master Feb 25, 2022
@jonasdn jonasdn deleted the jonasdn/doc-new-platform branch February 25, 2022 11:46
@ntamas
Copy link
Contributor

ntamas commented Feb 25, 2022

the idea we have that is for changing default values of parameters, you add your own platform

Seems OK to me, but does it mean that settings like CONFIG_PM_BAT_LOW_VOLTAGE, CONFIG_PM_BAT_CRITICAL_LOW_VOLTAGE etc (that I added in a previous PR) do not belong to the Kconfig menus any more?

@jonasdn
Copy link
Contributor Author

jonasdn commented Feb 25, 2022

the idea we have that is for changing default values of parameters, you add your own platform

Seems OK to me, but does it mean that settings like CONFIG_PM_BAT_LOW_VOLTAGE, CONFIG_PM_BAT_CRITICAL_LOW_VOLTAGE etc (that I added in a previous PR) do not belong to the Kconfig menus any more?

Correct! @tobbeanton will change them as part of his Bolt work.

@knmcguire knmcguire added this to the next-release milestone Mar 21, 2022
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.

5 participants