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

config definition tool can't build without config already generated #4946

Closed
mck1117 opened this issue Jan 6, 2023 · 10 comments
Closed

config definition tool can't build without config already generated #4946

mck1117 opened this issue Jan 6, 2023 · 10 comments
Assignees

Comments

@mck1117
Copy link
Member

mck1117 commented Jan 6, 2023

Currently unable to bootstrap a build with all generated files deleted (thinking about #4302).

Repro steps:

cd java_tools
rm ../java_console/models/src/main/java/com/rusefi/config/generated/Fields.java
./gradlew :config_definition:shadowJar

Fails with error (example, many similar trimmed):

image

Solution: config_definition shouldn't depend on any generated rusefi files.

@chuckwagoncomputing
Copy link
Member

Does anyone have an actual idea of how to solve this?
Maybe generate a text file that is parsed by ConfigGenerator at runtime?

e.g. Fields.COMPOSITE_PACKET_SIZE would be replaced with getField("COMPOSITE_PACKET_SIZE"), getFiled() being a method that parses the generated text file.

List of generated variables used, found with ./gradlew :config_definition:shadowJar 2>&1 | grep variable | sort | uniq | tr -s ' ' | cut -d ' ' -f 4 >list.txt

Fields
GAUGE_NAME_CLT
GAUGE_NAME_CPU_TEMP
GAUGE_NAME_DEBUG_F1
GAUGE_NAME_DEBUG_F2
GAUGE_NAME_DEBUG_F3
GAUGE_NAME_DEBUG_F4
GAUGE_NAME_DEBUG_F5
GAUGE_NAME_DEBUG_F6
GAUGE_NAME_DEBUG_F7
GAUGE_NAME_DEBUG_I1
GAUGE_NAME_DEBUG_I2
GAUGE_NAME_DEBUG_I3
GAUGE_NAME_ETB_DUTY
GAUGE_NAME_ETB_TARGET
GAUGE_NAME_FUEL_CRANKING
GAUGE_NAME_FUEL_PRESSURE_HIGH
GAUGE_NAME_FUEL_PRESSURE_LOW
GAUGE_NAME_FUEL_RUNNING
GAUGE_NAME_FUEL_VE
GAUGE_NAME_IAT
GAUGE_NAME_IDLE_POSITION
GAUGE_NAME_LAMBDA
GAUGE_NAME_MAF
GAUGE_NAME_MAP
GAUGE_NAME_RPM
GAUGE_NAME_TARGET_AFR
GAUGE_NAME_TCHARGE
GAUGE_NAME_THROTTLE_PEDAL
GAUGE_NAME_TIME
GAUGE_NAME_TPS
GAUGE_NAME_TRG_ERR
GAUGE_NAME_VBAT
GAUGE_NAME_VERSION
GAUGE_NAME_VVT_B1E
GAUGE_NAME_VVT_B2E
GAUGE_NAME_VVT_B2I
GAUGE_NAME_VVT_TARGET_B1E
GAUGE_NAME_VVT_TARGET_B1I
GAUGE_NAME_VVT_TARGET_B2E
GAUGE_NAME_VVT_TARGET_B2I
GAUGE_NAME_WARNING_LAST
GAUGE_NAME_WG_POSITION
PACK_MULT_ANGLE
PACK_MULT_FUEL_MASS
PACK_MULT_HIGH_PRESSURE
PACK_MULT_LAMBDA
PACK_MULT_MASS_FLOW
PACK_MULT_MS
PACK_MULT_PERCENT
PACK_MULT_PRESSURE
PACK_MULT_TEMPERATURE
PACK_MULT_VOLTAGE
PROTOCOL_ES_DOWN
PROTOCOL_ES_UP

@rusefillc
Copy link
Contributor

Does this have priority?

@chuckwagoncomputing
Copy link
Member

In my head it does, but I haven't been told so or anything. 🤷‍♂️
The main benefit of fixing this is that it helps enable #4302

@mck1117
Copy link
Member Author

mck1117 commented Feb 11, 2023

The main benefit of fixing this is that it helps enable #4302

yes, poking around with that one is how I discovered this.

@rusefillc
Copy link
Contributor

Yes, I have a proposal on how this should be address: we shall split configuration_definition code base and workflow a bit. See https://github.com/rusefi/rusefi/tree/master/java_tools/configuration_definition_base which was extracted with that specific idea in mind. While I've started I've definitely abandoned since, any chance @chuckwagoncomputing you would help?

Root cause: poor separation of responsibilities in java code, specifically configuration_definition should not depend on "models" it's too much coupling
Proper solution: improvement in java code architecture.

As of Jan 1st 2023 we had too much functionalities all piles together with quite a bunch of circular dependencies.

Specifically we have TriggerWheelInfo which is used by top level generation, and TriggerWheelInfo depends on rusefi_config.txt processing of

#define TRIGGER_COMMENT "#"
#define TRIGGER_GAPS_COUNT "gapsCount"
#define TRIGGER_GAP_FROM "gapFrom"

Actually that's totally actionable as atomic PR #5051

Once we exact "trivial" txt/c/java processing into separate step, we would move corresponding code into configuration_definition_base

Once we have trivial trigger generation in configuration_definition_base we would introduce new java module "trigger api generated" and have configuration_definition subproject depend on "trigger api generated" subproject/module not on "models"

TL,DR: configuration_definition should not depend on Fields.java

@mck1117
Copy link
Member Author

mck1117 commented Feb 11, 2023

Before we start throwing code at the wall, can we draw the dependency graph of what outputs actually depend on which inputs? I think others aren't clear on exactly what the circular-ish dependency currently is.

This would be helpful for other people who aren't inside @rusefillc's brain to be able to help.

@rusefillc
Copy link
Contributor

I am not a good human, I see a bit of discrepancy between ask for clarity above and mck1117/wideband#192 not being merged :(

Also
rusefi_txt > generator -> java -> WheelImage -> TRIGGER_SYNC_from Fields

@mck1117
Copy link
Member Author

mck1117 commented Feb 11, 2023

rusefi_txt > generator -> java -> WheelImage -> TRIGGER_SYNC_from Fields

but that has no loop - where is the part that the generator depends on something from Fields.java? I'm trying to narrow down whether it's a false dependency or an actual dependency.

Can we start with a narrower problem of something like "config generator should not build Sensor.java"?

@rusefillc rusefillc self-assigned this Feb 11, 2023
@rusefillc
Copy link
Contributor

Can we start with a narrower

Unfortunately https://github.com/rusefi/rusefi/wiki/Communication

@rusefillc
Copy link
Contributor

rusefillc commented Jun 18, 2023

Not great not terrible

cd java_tools
rm ../java_console/models/src/main/java/com/rusefi/config/generated/Fields.java
./gradlew :config_definition_base:shadowJar
cd ../firmware
./gen_live_documentation.sh
cd ../java_tools
./gradlew :config_definition:shadowJar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants