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

Convert schema into JSON #198

Merged
merged 5 commits into from
Dec 6, 2018
Merged

Convert schema into JSON #198

merged 5 commits into from
Dec 6, 2018

Conversation

jspectacular
Copy link
Collaborator

@jspectacular jspectacular commented Nov 14, 2018

Summary

Rebasing json schema with TIP master. Successor of jspectacular:json_schema branch, which was closed due to merge conflicts.

Dependencies

Install jsonschema, viz.:

sudo apt-get update
sudo apt-get install python3-jsonschema

Test Plan

Verify the builds

Note: old schema.c file should not be in schema folder and has been removed from repo. It will cause build failure. auto_schema.c included in repo for those still using Code Composer, but file will be generated from json schema declaration.

  1. Objective: verify ec build.
    With fresh checkout, build from firmware/ec. Verify build with no new errors or warnings vs master; verify that output dialog says:

Generating auto_schema.c because validation file does not exist

  1. Objective: verify host build
    Build from firmware/host. Verify build; verify that output dialog says:

auto_schema.c does not need an update

  1. Objective: verify that new auto schema file is generated when source json configuration is updated
    Touch sys_schema.json, build from either host or ec. Verify that output dialog says

Generating auto_schema.c because sys_schema.json has been updated

  1. Objective: verify that new auto schema file is generated when json validation file is missing
    Delete valid_schema.json, build from either host or ec. Verify that output dialog says

Generating auto_schema.c because validation file does not exist

  1. Objective: verify that build fails when json configuration file is missing
    Delete sys_schema.json, build from either host or ec. Verify that build fails and output dialog says

ERROR: sys_schema.json does not exist

Verify the structured data tester and generator

Note: cd to firmware/utilities/schema.
Note: Draft 6 validator only available with jsonschema 3.0; use draft 4 for now

  1. Objective: Demonstrate how json configuration file can be created from schema c-file
    Delete auto_schema.c and sys_schema.json; copy old schema.c into folder. Generate new JSON schema file (ex4 in sdtester.py), from shell: python3 sdtester.py -g; verify that sys_schema.json is created

  2. Objective: Demonstrate how schema c-file is generated from json configuration file
    From shell, repeat ex1 in sdtester.py. Verify that auto_schema.c is created

  3. Objective: Demonstrate that sdtester fails when json configuration file is missing.
    From shell, delete sys_schema.json and attempt to repeat step 7; verify system exit

  4. Objective: Demonstrate that sdtester throws validation error when meta schema is bad.
    Uncomment line 107 (only) in schemautils.py and repeat test 7; verify that output dialog states

Error in meta-schema: ...

  1. Objective: Demonstrate that sdtester throws validation error when configuration schema is bad.
    Uncomment line 108 (only) in schemautils.py and repeat test 7; verify that output dialog states

Error in schema configuration: ...

  1. Objective: Demonstrate that sdtester will validate any valid json configuration with draft option d0.
    Modify sys_schema.json as desired, run example 5 in sdtester.py with d0 as the validation file. Verify that the c-file is created.

Issues

Closes issue #173
Closes issue #180

@ghost
Copy link

ghost commented Nov 14, 2018

Can one of the admins verify this patch?

@jspectacular
Copy link
Collaborator Author

For old thread, see #180

@jspectacular jspectacular self-assigned this Nov 14, 2018
@JoshuaJeyaraj
Copy link
Collaborator

@Dineshraghuwanshi please verify if the json file includes equivalent changes for #164

@jspectacular
Copy link
Collaborator Author

@Dineshraghuwanshi please verify if the json file includes equivalent changes for #164

Can also check if auto_schema.c shows the changes.

Copy link
Collaborator

@JoshuaJeyaraj JoshuaJeyaraj left a comment

Choose a reason for hiding this comment

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

Any change in the JSON file will get reflected in the schema.c file , however the reverse is not true.
For users/developers who prefer to modify .c file directly i see that $ python3 sdtester.py -g -c ../../ec/platform/oc-sdr/schema/schema.c -j ../../ec/platform/oc-sdr/schema/sys_schema.json this command will generate the JSON file . Is there any plan to document this ? .

This is a major change could we test on the actual target and confirm that nothing is broken from host and firmware perspective.

@JoshuaJeyaraj
Copy link
Collaborator

@Dineshraghuwanshi please verify if the json file includes equivalent changes for #164

Can also check if auto_schema.c shows the changes.

This looks good to me . . . .

@ghost
Copy link

ghost commented Nov 16, 2018

For users/developers who prefer to modify .c file directly

Eventually the .c will be removed from the repo, so users shouldn't modify the .c directly after this change. Before it can be removed, CCS project has to be modified to run the generation script before compile, which will be done in a subsequent PR.
The header of auto_schema.c has a warning not to edit the file since it is auto-generated. We should include this info in the wiki somewhere, too.

This is a major change could we test on the actual target and confirm that nothing is broken from host and firmware perspective.

Agree, this should be verified on the hardware. In theory the binary files shouldn't really change at all, curious the result of running diff on files created using auto_schema.c vs. original schema.c. There might be just enough restructuring that it changes, though...

@jspectacular
Copy link
Collaborator Author

The schema c-file is going away and at this point it no longer needs to be in the repo. It was left in for 2 reasons: First, some people use CCS and the build process isn't set up to run Python from within CCS. Issue #203 was submitted to resolve this. The second reason is that having the schema c-file in this branch makes the code review easier. The purpose of this change is that, going forward, users will modify the schema directly in the JSON configuration file. Once issue 203 is resolved, the schema c-file will be removed, as well as the sdtester code that converts a c-file into JSON.

@jspectacular
Copy link
Collaborator Author

Any more changes? We're targeting a merge this week.

@JoshuaJeyaraj
Copy link
Collaborator

Do we have any plan to test on the target platform ?

@jspectacular
Copy link
Collaborator Author

Added step 11 to test plan to confirm that we can handle arbitrary json configurations.

@jspectacular
Copy link
Collaborator Author

Do we have any plan to test on the target platform ?

Yes, we should

@JoshuaJeyaraj
Copy link
Collaborator

Please add the test logs once they are done , i will review those and approve this PR .

@jspectacular
Copy link
Collaborator Author

Please add the test logs once they are done , i will review those and approve this PR .

We don't have a complete test setup, so most tests will fail regardless. Is your team able to test?

@JoshuaJeyaraj
Copy link
Collaborator

We are in the middle of the testing alerts, i doubt if we will be able to test immediately .
Can you please mention which all can be tested from your side , i will check if we can test the rest .

@jspectacular
Copy link
Collaborator Author

jspectacular commented Nov 27, 2018

We are in the middle of the testing alerts, i doubt if we will be able to test immediately .
Can you please mention which all can be tested from your side , i will check if we can test the rest .

Vishal can test for us at MPK and approve when it passes. Thanks.

@vthakur7f
Copy link
Collaborator

We are in the middle of the testing alerts, i doubt if we will be able to test immediately .
Can you please mention which all can be tested from your side , i will check if we can test the rest .

Vishal can test for us at MPK and approve when it passes. Thanks.

RF Tested and verified OK. Attached are few screenshots for attenuation and PA controls.
TRX attenuation is set to 10 dBm and External attenuation used is 20 dBm. Both chains tested.

img_1309
img_e1310
img_1311
img_e1312
img_1313
img_e1314
img_1315
img_e1316

Copy link
Collaborator

@vthakur7f vthakur7f left a comment

Choose a reason for hiding this comment

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

+1

@jspectacular jspectacular merged commit 9b3e785 into master Dec 6, 2018
@jspectacular
Copy link
Collaborator Author

Thanks to all for the effort reviewing, feedback, and help testing. This one was a major change.

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.

3 participants