Optimize Addresses.h generation and fix incorrect addresses for certain output types like DcsBios::LED #240

maciekish · 2023-09-23T11:56:18Z

Recreate Addresses.h in init() on each run, completely replacing it and avoiding stale entries.
Fixes incorrect parameters for certain output functions like DcsBios::LED, DcsBios::ServoOutput etc

…ale entries. Also only writes the file once speeding up the whole process greatly.

…ript by adding _ADDR

charliefoxtwo · 2023-09-23T22:49:44Z

I think it's probably fine to have both entries in the json - I initially didn't fully understand why they were there.

However, I'm not super familiar with the arduino stuff so I'd like to better understand why both values are needed to begin with. Additionally, I'd suggest reading the values from the documentation where they've already been defined instead of recomputing them in the save call:

local full_identifier = BIOS.util.addressDefineIdentifier(moduleName, identifier)

could be

local full_identifier = output.address_identifier

maciekish · 2023-09-24T09:29:13Z

I think it's probably fine to have both entries in the json - I initially didn't fully understand why they were there.

However, I'm not super familiar with the arduino stuff so I'd like to better understand why both values are needed to begin with. Additionally, I'd suggest reading the values from the documentation where they've already been defined instead of recomputing them in the save call:
local full_identifier = BIOS.util.addressDefineIdentifier(moduleName, identifier)
could be
local full_identifier = output.address_identifier

It’s fine but redundant, since i’ve already made the change its better like this, saves a bit of space. The only caveat is that if anyone changes the _ADDR suffix for any reason they have to be smart and seach the whole project for _ADDR and make sure they get them all.

Regarding the recomputation, do you mean in Util.lua or Protocol.lua? I’m not at the computer right now but i don’t think the documentation output is accessible at that point. Anyway the recomputation is stable and fast, and only runs in „developer mode” anyway. I don’t believe this is an issue, but if you want i can look into it later?

maciekish · 2023-09-24T11:20:59Z

@charliefoxtwo I see what you mean now, it was immediately obvious when i opened it on a big screen. However, there are addresses which are undocumented and only using output.address_identifier drops about 390Kb of addresses from the file, so i changed it to address_identifier = output.address_identifier or BIOS.util.addressDefineIdentifier(moduleName, identifier). It will use the existing identifier if possible, otherwise compute one.

charliefoxtwo · 2023-09-24T16:15:11Z

Since the address only is used in the reference tool and will alps be added to bort (an external project), I suggest having both in the json files. Otherwise even if everything in the bios project is changed, bort will still need separate changes (same for the external ctrl ref tool).

Regarding recomputing, it's not performance I'm concerned about, just consistency. I'll note that if the json documentation doesn't have an address_identifier then it won't display in the reference tool either, so recomputing probably doesn't offer much value (it would add the entry, sure but that entry would be completely undocumented and the user would have no idea it exists). In my module refactor I've made sure every output will have those items added, but in the meantime some modules define new outputs inline so it's definitely possible some won't have one.

maciekish · 2023-09-24T16:38:34Z

@charliefoxtwo I completely forgot about Bort today, you're right. Let's put the address_only_idenfitier back. I think thats why i did it like this initially, i remember we were talking about Bort a few weeks back.

Regarding the undocumented addresses, should we keep it this way until the refactoring is complete?

…n javascript by adding _ADDR" This reverts commit c2f1917.

maciekish · 2023-09-24T16:41:00Z

Done ↑

charliefoxtwo · 2023-09-24T16:49:31Z

Regarding the undocumented addresses, should we keep it this way until the refactoring is complete?

It's fine either way 🤷 I'm happy with the changes you've made which at least check to see if the documentation exists

maciekish · 2023-09-24T16:50:12Z

I'd leave it as is then, i feel uneasy with stuff disappearing. This should all be done and ready in that case.

Scripts/DCS-BIOS/lib/Protocol.lua

maciekish · 2023-09-24T18:57:47Z

Turns out it was simpler than i thought, i have refactored it to make it self-contained now.

charliefoxtwo

Looks good to me!

charliefoxtwo · 2023-09-24T21:44:10Z

@maciekish this isn't necessarily a new issue but something I just noticed from your previous PR:
address_identifier contains the address, mask, and shift_by values. However, DcsBios::LED uses only address and mask - no shift_by. Is that going to cause an issue on the substitution?

maciekish · 2023-09-24T21:48:18Z

@maciekish this isn't necessarily a new issue but something I just noticed from your previous PR:

address_identifier contains the address, mask, and shift_by values. However, DcsBios::LED uses only address and mask - no shift_by. Is that going to cause an issue on the substitution?

Oh boy, yeah that will definitely cause a compile error. I don’t use the ::LED, ::Servo etc functions so i missed this 🤦🏻 Will have to look at it tomorrow as its past midnight here now. Is it possible to run test with a C compiler on Github as well? Or at least a linter?

charliefoxtwo · 2023-09-24T21:55:41Z

No worries.

I'm not sure what sort of compiler we'd run or how we'd set it up, but it's not out of the question. If you can come up with a github action that will validate that stuff, I'm fine with it. But I'm not sure you'll be able to, since the documentation doesn't have any record of how those constants will be used, and documentation tools could display them any number of ways.

If you come up with something, let me know.

maciekish · 2023-09-25T11:46:36Z

No worries.

I'm not sure what sort of compiler we'd run or how we'd set it up, but it's not out of the question. If you can come up with a github action that will validate that stuff, I'm fine with it. But I'm not sure you'll be able to, since the documentation doesn't have any record of how those constants will be used, and documentation tools could display them any number of ways.

If you come up with something, let me know.

I have identified the issue and will submit a PR shortly.

maciekish · 2023-09-25T12:08:41Z

Actually since the changes include the optimized addresses.h generation in this PR, i have added the fix to this PR. I identified the following output types:

Suffix	Type			Parameters		Condition		Default
(None)	IntegerBuffer		addr, mask, sb		integer			DcsBios::IntegerBuffer masterCautionBuffer(0x1012, 0x0800, 11, onMasterCautionChange);
_AM	LED			addr, mask		integer, max 1		DcsBios::LED masterCaution(0x1012, 0x0800, PIN);
_A	ServoOutput		addr			integer, max 65535	DcsBios::ServoOutput cabinPressAlt(0x1134, PIN, 544, 2400);
_A	StringBuffer		addr			string			DcsBios::StringBuffer<24> cduLine0Buffer(0x11c0, onCduLine0Change);
(None)	FloatBuffer		addr, mask, sb		float			DcsBios::FloatBuffer engineRPM(0x1012, 0x0800, 11, 0, 256);

Which result in the following three defines to support all cases:

#define CONTROL_NAME = 0x1012, 0x800, 11
#define CONTROL_NAME_A = 0x1012
#define CONTROL_NAME_AM = 0x1012, 0x800

They are only generated as required based on the control type, for example there is no A_10C_MASTER_CAUTION_A as only A_10C_MASTER_CAUTION and A_10C_MASTER_CAUTION_AM is used by DcsBios::IntegerBuffer and DcsBios::LED. Here is the code that decides which #define to add to Addresses.h: https://github.com/DCSBIOSKit/dcs-bios/blob/da92fb378b0e8d332043e71aa7bb8902a4341794/Scripts/DCS-BIOS/lib/Protocol.lua#L107-L121

I have also updated the json files and the js to use the correct address.

Scripts/DCS-BIOS/lib/Protocol.lua

maciekish · 2023-09-26T20:26:08Z

Anything else to fix here?

charliefoxtwo · 2023-09-26T21:45:42Z

can we just remove the float bit? I don't think we should leave that around since it kind of implies we should be doing something there when in fact we don't.

maciekish · 2023-09-27T19:11:40Z

can we just remove the float bit? I don't think we should leave that around since it kind of implies we should be doing something there when in fact we don't.

Sure, thats kind of the reason i added it as well. I'll get it updated tomorrow.

maciekish · 2023-09-28T15:16:58Z

Done, i skimmed through the conflicts it looks like indentation only but not sure how to properly resolve them in Github.

jdahlblom · 2023-09-28T15:22:48Z

Just read about this and if there is no resolve button on this page the conflicts are considered too complex to be solved from here.

maciekish · 2023-09-28T15:25:08Z

Since i don't have write access, it looks like i cant do anything about them. I could try downloading and committing the current A-4E-C files to just resolve the conflicts but you would have to re-run the export as i don't have access to DCS for 2 days and by that time we may have more conflicts...

charliefoxtwo · 2023-09-28T15:57:36Z

You should merge master into this branch locally, resolve the conflicts locally, and then push the changes. That should take care of it.

I suspect the conflicts are in the json files, so you'll probably need to regenerate them.

maciekish · 2023-09-28T15:58:49Z

I thought that will mess up the PR, but i'll give it a try. I can't regenerate the files until tomorrow evening at the earliest though.

charliefoxtwo · 2023-09-28T17:54:27Z

Merging the target branch into the source branch to keep it up to date is totally normal behavior :)

maciekish · 2023-09-30T10:29:29Z

Done

charliefoxtwo

Just the one item needs to be changed for cross-OS compatibility on the local compile, everything else looks good

Scripts/DCS-BIOS/lib/Protocol.lua

Co-authored-by: Charlie <30303272+charliefoxtwo@users.noreply.github.com>

maciekish · 2023-09-30T21:20:13Z

I updated the path but Github is still complaining..

charliefoxtwo · 2023-09-30T21:23:49Z

I updated the path but Github is still complaining..

Is it? everything looks good to me! If you're good with it, I'm happy to merge this

maciekish · 2023-10-01T08:51:45Z

It was for a few minutes then it looked the same as yours. Thanks for all the input 👍🏻

maciekish added 2 commits September 23, 2023 14:47

Recreate the Addresses.h from scratch every time DCS runs to avoid st…

0ce6966

…ale entries. Also only writes the file once speeding up the whole process greatly.

Remove the extraneous address_only_identifier and derive it in javasc…

c2f1917

…ript by adding _ADDR

Use existing output.address_identifier if present

50f9d14

Revert "Remove the extraneous address_only_identifier and derive it i…

d43da00

…n javascript by adding _ADDR" This reverts commit c2f1917.

charliefoxtwo reviewed Sep 24, 2023

View reviewed changes

Scripts/DCS-BIOS/lib/Protocol.lua Outdated Show resolved Hide resolved

Refactored saveAddresses() to make it selfcontained and run in init()

b3252f3

saveAddresses() should probably only run in dev mode like saveDoc()

e4dad3d

charliefoxtwo previously approved these changes Sep 24, 2023

View reviewed changes

maciekish changed the title ~~Optimize Addresses.h generation and remove redundant address_only_identifier from json~~ Optimize Addresses.h generation Sep 24, 2023

charliefoxtwo self-requested a review September 24, 2023 21:56

Fix incorrect address definitions and update Addresses.h and json files

da92fb3

maciekish changed the title ~~Optimize Addresses.h generation~~ Optimize Addresses.h generation and fix incorrect addresses for certain output types like DcsBios::LED Sep 25, 2023

charliefoxtwo reviewed Sep 25, 2023

View reviewed changes

Scripts/DCS-BIOS/lib/Protocol.lua Outdated Show resolved Hide resolved

Removed the "float" output type as it doesn't seem to be used anywhere

4d0ea53

maciekish dismissed charliefoxtwo’s stale review via 4d0ea53 September 30, 2023 04:47

maciekish added 2 commits September 30, 2023 13:24

Merge remote-tracking branch 'upstream/master'

2042302

Regenerated conflicting files in upstream

651b2fc

Merge branch 'master' into master

f1da1a8

charliefoxtwo requested changes Sep 30, 2023

View reviewed changes

Scripts/DCS-BIOS/lib/Protocol.lua Outdated Show resolved Hide resolved

maciekish and others added 2 commits October 1, 2023 00:17

Update Protocol.lua

c10ddf0

Co-authored-by: Charlie <30303272+charliefoxtwo@users.noreply.github.com>

Merge branch 'master' into master

cd1d2b3

charliefoxtwo approved these changes Sep 30, 2023

View reviewed changes

charliefoxtwo merged commit 96b3f9e into DCS-Skunkworks:master Oct 1, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize Addresses.h generation and fix incorrect addresses for certain output types like DcsBios::LED #240

Optimize Addresses.h generation and fix incorrect addresses for certain output types like DcsBios::LED #240

maciekish commented Sep 23, 2023 •

edited

Loading

charliefoxtwo commented Sep 23, 2023

maciekish commented Sep 24, 2023

maciekish commented Sep 24, 2023

charliefoxtwo commented Sep 24, 2023

maciekish commented Sep 24, 2023 •

edited

Loading

maciekish commented Sep 24, 2023

charliefoxtwo commented Sep 24, 2023 •

edited

Loading

maciekish commented Sep 24, 2023

maciekish commented Sep 24, 2023

charliefoxtwo left a comment

charliefoxtwo commented Sep 24, 2023

maciekish commented Sep 24, 2023 •

edited

Loading

charliefoxtwo commented Sep 24, 2023

maciekish commented Sep 25, 2023 •

edited

Loading

maciekish commented Sep 25, 2023 •

edited

Loading

maciekish commented Sep 26, 2023

charliefoxtwo commented Sep 26, 2023

maciekish commented Sep 27, 2023 •

edited

Loading

maciekish commented Sep 28, 2023

jdahlblom commented Sep 28, 2023

maciekish commented Sep 28, 2023

charliefoxtwo commented Sep 28, 2023

maciekish commented Sep 28, 2023

charliefoxtwo commented Sep 28, 2023

maciekish commented Sep 30, 2023

charliefoxtwo left a comment

maciekish commented Sep 30, 2023 •

edited

Loading

charliefoxtwo commented Sep 30, 2023

maciekish commented Oct 1, 2023

Optimize Addresses.h generation and fix incorrect addresses for certain output types like DcsBios::LED #240

Optimize Addresses.h generation and fix incorrect addresses for certain output types like DcsBios::LED #240

Conversation

maciekish commented Sep 23, 2023 • edited Loading

charliefoxtwo commented Sep 23, 2023

maciekish commented Sep 24, 2023

maciekish commented Sep 24, 2023

charliefoxtwo commented Sep 24, 2023

maciekish commented Sep 24, 2023 • edited Loading

maciekish commented Sep 24, 2023

charliefoxtwo commented Sep 24, 2023 • edited Loading

maciekish commented Sep 24, 2023

maciekish commented Sep 24, 2023

charliefoxtwo left a comment

Choose a reason for hiding this comment

charliefoxtwo commented Sep 24, 2023

maciekish commented Sep 24, 2023 • edited Loading

charliefoxtwo commented Sep 24, 2023

maciekish commented Sep 25, 2023 • edited Loading

maciekish commented Sep 25, 2023 • edited Loading

maciekish commented Sep 26, 2023

charliefoxtwo commented Sep 26, 2023

maciekish commented Sep 27, 2023 • edited Loading

maciekish commented Sep 28, 2023

jdahlblom commented Sep 28, 2023

maciekish commented Sep 28, 2023

charliefoxtwo commented Sep 28, 2023

maciekish commented Sep 28, 2023

charliefoxtwo commented Sep 28, 2023

maciekish commented Sep 30, 2023

charliefoxtwo left a comment

Choose a reason for hiding this comment

maciekish commented Sep 30, 2023 • edited Loading

charliefoxtwo commented Sep 30, 2023

maciekish commented Oct 1, 2023

maciekish commented Sep 23, 2023 •

edited

Loading

maciekish commented Sep 24, 2023 •

edited

Loading

charliefoxtwo commented Sep 24, 2023 •

edited

Loading

maciekish commented Sep 24, 2023 •

edited

Loading

maciekish commented Sep 25, 2023 •

edited

Loading

maciekish commented Sep 25, 2023 •

edited

Loading

maciekish commented Sep 27, 2023 •

edited

Loading

maciekish commented Sep 30, 2023 •

edited

Loading