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

cars: introduce "platformconfig" to be a configuration for all relevant car information #31542

Merged
merged 5 commits into from
Feb 22, 2024

Conversation

jnewb1
Copy link
Contributor

@jnewb1 jnewb1 commented Feb 22, 2024

No description provided.

@jnewb1 jnewb1 added refactor car vehicle-specific labels Feb 22, 2024
@sshane
Copy link
Contributor

sshane commented Feb 22, 2024

Why not use CarInfo?

@jnewb1
Copy link
Contributor Author

jnewb1 commented Feb 22, 2024

Why not use CarInfo?

I feel like a "car" is different from a "platform", a platform can support multiple different cars

@jnewb1
Copy link
Contributor Author

jnewb1 commented Feb 22, 2024

And CarInfo is already very dense with documentation stuff, I feel like putting the openpilot configuration in there makes it way too big

Copy link
Contributor

@adeebshihadeh adeebshihadeh left a comment

Choose a reason for hiding this comment

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

I like the abstraction and iterative approach. After merging this, we can do a bigger one, like Toyota, and see if we want to adjust the abstraction before touching too many other files.

Comment on lines 40 to 41
CAR_INFO = {p.platform_str: p.car_info for p in CAR}
DBC = {p.platform_str: p.dbc_dict for p in CAR}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe handle this in get_interface_attr?

@adeebshihadeh
Copy link
Contributor

Why not use CarInfo?

I think we want to maintain a separate struct for docs, but perhaps we rename it to be more specific?

Comment on lines 22 to 27
class CAR(Platforms):
BODY = (
"COMMA BODY",
CarInfo("comma body", package="All"),
dbc_dict('comma_body', None),
)
Copy link
Contributor

@sshane sshane Feb 22, 2024

Choose a reason for hiding this comment

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

ah yes this is what I sort of meant since you need multiple CarInfo's per platform. I would just make it a list or tuple by definition so there's no dynamic type checking on the CarInfo part anymore eh, that's really separate from this

class Platforms(str, ReprEnum):
config: PlatformConfig

def __new__(cls, platform_config: PlatformConfig):
Copy link
Contributor Author

@jnewb1 jnewb1 Feb 22, 2024

Choose a reason for hiding this comment

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

this bit of hackery allows CAR within values.py to still be a StrEnum for backwards compatibility, until we fully move everything to the new PlatformConfig. CAR.BODY.config will be the PlatformConfig

for example, the fingerprints file and everything that references it currently depends on the fact the CAR.BODY is a string

FINGERPRINTS = {
  CAR.BODY: [{
    513: 8, 516: 8, 514: 3, 515: 4
  }],
}

Copy link
Contributor Author

@jnewb1 jnewb1 Feb 22, 2024

Choose a reason for hiding this comment

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

this allows this PR's fingerprint to be small and we can make these changes in small batches, while maintaining the interface that we want

Comment on lines +40 to +41
CAR_INFO = CAR.create_carinfo_map()
DBC = CAR.create_dbc_map()
Copy link
Contributor

Choose a reason for hiding this comment

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

can we entirely skip these lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DBC is imported directly from values.py in a few places, was trying to make this a minimal PR

Copy link
Contributor

Choose a reason for hiding this comment

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

the plan is to remove these once everything is moved over I'm assuming

Copy link
Contributor Author

@jnewb1 jnewb1 Feb 23, 2024

Choose a reason for hiding this comment

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

the plan is to remove these once everything is moved over I'm assuming

yes, everything should directly reference the PlatformConfig when we are done

Comment on lines +246 to +248
CarInfos = Union[CarInfo, List[CarInfo]]

@dataclass(order=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

two lines

Comment on lines +79 to +81
DbcDict = Dict[str, str]

def dbc_dict(pt_dbc, radar_dbc, chassis_dbc=None, body_dbc=None) -> DbcDict:
Copy link
Contributor

Choose a reason for hiding this comment

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

two lines

Comment on lines +40 to +41
CAR_INFO = CAR.create_carinfo_map()
DBC = CAR.create_dbc_map()
Copy link
Contributor

Choose a reason for hiding this comment

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

the plan is to remove these once everything is moved over I'm assuming

@jnewb1 jnewb1 merged commit 1ee8c9a into master Feb 22, 2024
28 checks passed
@jnewb1 jnewb1 deleted the platform-config branch February 22, 2024 23:58
@jnewb1 jnewb1 mentioned this pull request Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
car vehicle-specific refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants