Skip to content

Fix dictionary JSON struct default values #931

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.

Sign up for GitHub

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

Open
jwest115 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix dictionary JSON struct default values #931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions compiler/lib/src/main/scala/codegen/DictionaryJsonWriter/DictionaryJsonEncoder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,17 +169,22 @@ case class DictionaryJsonEncoder(

/** JSON Encoding for arrays */
def arrayElementsAsJson(elements: List[Value]): Json = {
val arrayRes = for(e <- elements) yield {
val res = e match {
// Case where array is N-dimensional
case Value.Array(a, t) => arrayElementsAsJson(a.elements)
case _ => valueAsJson(e)
}

res.asJson
}
val arrayRes = for(e <- elements) yield valueAsJson(e)
Copy link
Collaborator

@bocchino bocchino Feb 27, 2026

Choose a reason for hiding this comment

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

This looks like a code improvement (eliminating a special case). Can we keep this change and preserve the existing behavior?

arrayRes.asJson
}

Copy link
Collaborator

@bocchino bocchino Feb 27, 2026

Choose a reason for hiding this comment

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

As discussed in issue #925, after further discussion, I think we should revert this change. The current behavior is correct according to the way that FPP represents struct values with member arrays. However, @jwest115 can you make sure that the current F Prime JSON spec is consistent with the FPP model and the generated JSON here? This is a tricky case.

Copy link
Collaborator

@bocchino bocchino Mar 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Now I'm not so sure if we should revert. I think the root cause of the issue is that the dictionary JSON is underspecified here. I opened an issue on this: nasa/fprime#4787. There are at least two ways this could work:

  1. When a struct member of type T has a size n, the value stored in the member must be a JSON array with n elements, each of type T. When there is no size, the value must be a single value of type T.

  2. Regardless of whether the struct member has a size, the value stored in the member can be a single value or an array. If there is no size and an array of elements, the array size must be one. If there is a size and a single element, the single element is stored in every position in the array.

Probably we want option (1), but the JSON spec does not specify what behavior is intended. We should tighten up the JSON spec first, and then make the fpp-to-json output conform to it.

/** JSON Encoding for struct values */
def structValueAsJson(value: Value.Struct): Json = {
val Value.Struct(Value.AnonStruct(members), t) = value
val Type.Struct(_, _, _, sizes, _) = dictionaryState.a.typeMap(t.node._2.id)
members.map((key, v) =>
val valueJson = valueAsJson(v)
sizes.getOrElse(key, 1) match
case size if size > 1 =>
(key.toString -> Json.fromValues(List.fill(size)(valueJson)))
case _ => (key.toString -> valueJson)
).asJson
}

/** JSON Encoding for FPP Values
*
Expand All @@ -197,7 +202,7 @@ case class DictionaryJsonEncoder(
val qualifiedName = dictionaryState.a.getQualifiedName(Symbol.Enum(t.node)).toString
s"${qualifiedName}.${v._1}".asJson // FQN of the enum constant
}
case Value.Struct(Value.AnonStruct(members), t) => members.map((key, value) => (key.toString -> valueAsJson(value))).asJson
Copy link
Collaborator

@bocchino bocchino Feb 26, 2026

Choose a reason for hiding this comment

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

As noted, the original code looks more correct here. The new code seems to be a special case to implement a difference between FPP semantics and GDS semantics. I'd rather keep those the same, if possible.

case v: Value.Struct => structValueAsJson(v)
case _ => value.toString.asJson
}
}
Expand Down Expand Up @@ -280,7 +285,7 @@ case class DictionaryJsonEncoder(
"members" -> membersFormatted.toMap.asJson,
)
val optionalValues = Map(
"default" -> default,
"default" -> default.map(structValueAsJson),
"annotation" -> concatAnnotations(preA, postA)
)
jsonWithOptionalValues(json, optionalValues)
Expand Down
157 changes: 91 additions & 66 deletions compiler/tools/fpp-to-dict/test/top/BasicDpTopologyDictionary.ref.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,37 +8,6 @@
"dictionarySpecVersion" : "1.0.0"
},
"typeDefinitions" : [
{
"kind" : "struct",
"qualifiedName" : "FppTest.DpTestComponent.Complex",
"members" : {
"f1" : {
"type" : {
"name" : "FppTest.DpTestComponent.Data",
"kind" : "qualifiedIdentifier"
},
"index" : 0,
"annotation" : "A struct in the struct"
},
"f2" : {
"type" : {
"name" : "U32",
"kind" : "integer",
"size" : 32,
"signed" : false
},
"index" : 1,
"annotation" : "A simple U32 field"
}
},
"default" : {
"f1" : {
"u16Field" : 0
},
"f2" : 0
},
"annotation" : "Data for a ComplexRecord"
},
{
"kind" : "alias",
"qualifiedName" : "FwTlmPacketizeIdType",
Expand Down Expand Up @@ -83,6 +52,55 @@
],
"default" : "Fw.DpState.UNTRANSMITTED"
},
{
"kind" : "struct",
"qualifiedName" : "FppTest.DpTestComponent.Complex",
"members" : {
"f1" : {
"type" : {
"name" : "FppTest.DpTestComponent.Data",
"kind" : "qualifiedIdentifier"
},
"index" : 0,
"size" : 2,
"annotation" : "A struct in the struct"
},
"f2" : {
"type" : {
"name" : "U32",
"kind" : "integer",
"size" : 32,
"signed" : false
},
"index" : 1,
"annotation" : "A simple U32 field"
}
},
"default" : {
"f1" : [
{
"u16Field" : [
10,
10,
10,
10,
10
]
},
{
"u16Field" : [
10,
10,
10,
10,
10
]
}
],
"f2" : 0
},
"annotation" : "Data for a ComplexRecord"
},
{
"kind" : "alias",
"qualifiedName" : "BaseIdType",
Expand Down Expand Up @@ -206,6 +224,31 @@
"signed" : false
}
},
{
"kind" : "struct",
"qualifiedName" : "FppTest.DpTestComponent.Data",
"members" : {
"u16Field" : {
"type" : {
"name" : "FppTest.DpTestComponent.AliasU16",
"kind" : "qualifiedIdentifier"
},
"index" : 0,
"size" : 5,
"annotation" : "A U16 field"
}
},
"default" : {
"u16Field" : [
10,
10,
10,
10,
10
]
},
"annotation" : "Data for a DataRecord"
},
{
"kind" : "alias",
"qualifiedName" : "FwDpIdType",
Expand All @@ -220,6 +263,23 @@
"signed" : false
}
},
{
"kind" : "alias",
"qualifiedName" : "FwTimeBaseStoreType",
"type" : {
"name" : "U16",
"kind" : "integer",
"size" : 16,
"signed" : false
},
"underlyingType" : {
"name" : "U16",
"kind" : "integer",
"size" : 16,
"signed" : false
},
"annotation" : "The type used to serialize a time base value"
},
{
"kind" : "alias",
"qualifiedName" : "FwChanIdType",
Expand Down Expand Up @@ -275,41 +335,6 @@
"size" : 32,
"signed" : false
}
},
{
"kind" : "alias",
"qualifiedName" : "FwTimeBaseStoreType",
"type" : {
"name" : "U16",
"kind" : "integer",
"size" : 16,
"signed" : false
},
"underlyingType" : {
"name" : "U16",
"kind" : "integer",
"size" : 16,
"signed" : false
},
"annotation" : "The type used to serialize a time base value"
},
{
"kind" : "struct",
"qualifiedName" : "FppTest.DpTestComponent.Data",
"members" : {
"u16Field" : {
"type" : {
"name" : "FppTest.DpTestComponent.AliasU16",
"kind" : "qualifiedIdentifier"
},
"index" : 0,
"annotation" : "A U16 field"
}
},
"default" : {
"u16Field" : 0
},
"annotation" : "Data for a DataRecord"
}
],
"constants" : [
Expand Down
86 changes: 47 additions & 39 deletions compiler/tools/fpp-to-dict/test/top/DictionaryDefsTopologyDictionary.ref.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,53 @@
"signed" : false
}
},
{
"kind" : "struct",
"qualifiedName" : "S",
"members" : {
"X" : {
"type" : {
"name" : "string",
"kind" : "string",
"size" : 256
},
"index" : 0
},
"Y" : {
"type" : {
"name" : "A2",
"kind" : "qualifiedIdentifier"
},
"index" : 1,
"size" : 2
},
"Z" : {
"type" : {
"name" : "S2",
"kind" : "qualifiedIdentifier"
},
"index" : 2
}
},
"default" : {
"X" : "",
"Y" : [
[
0,
0,
0
],
[
0,
0,
0
]
],
"Z" : {
"X" : 0
}
}
},
{
"kind" : "alias",
"qualifiedName" : "FwOpcodeType",
Expand Down Expand Up @@ -306,45 +353,6 @@
"signed" : false
}
},
{
"kind" : "struct",
"qualifiedName" : "S",
"members" : {
"X" : {
"type" : {
"name" : "string",
"kind" : "string",
"size" : 256
},
"index" : 0
},
"Y" : {
"type" : {
"name" : "A2",
"kind" : "qualifiedIdentifier"
},
"index" : 1
},
"Z" : {
"type" : {
"name" : "S2",
"kind" : "qualifiedIdentifier"
},
"index" : 2
}
},
"default" : {
"X" : "",
"Y" : [
0,
0,
0
],
"Z" : {
"X" : 0
}
}
},
{
"kind" : "alias",
"qualifiedName" : "FwSizeType",
Expand Down
6 changes: 4 additions & 2 deletions compiler/tools/fpp-to-dict/test/top/dataProducts.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@ module FppTest {
@ Data for a DataRecord
struct Data {
@ A U16 field
u16Field: AliasU16
u16Field: [5] AliasU16
} default {
u16Field = 10
}

@ Data for a ComplexRecord
struct Complex {
@ A struct in the struct
f1: Data
f1: [2] Data
@ A simple U32 field
f2: U32
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/tools/fpp-to-dict/test/top/dictionaryDefs.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ dictionary enum E {

dictionary struct S {
X: string,
Y: A2,
Y: [2] A2,
Z: S2
}

Expand Down Expand Up @@ -41,7 +41,7 @@ enum E3 {
}

struct S3 {
X: string
X: [5] string
}

constant C3 = 1
Expand Down