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

Define a IM/DM decoupling interface for interaction model engine to interact with the data model bits #32914

Merged
merged 61 commits into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from 55 commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
5697b48
Start adding a interaction model decoupling
andy31415 Apr 9, 2024
2a59741
Restyle
andy31415 Apr 9, 2024
d23acfa
Also add invoke responder bits
andy31415 Apr 9, 2024
67c6d29
Add the first model definition, for each operation
andy31415 Apr 9, 2024
a582d72
Add all files into the build definition
andy31415 Apr 9, 2024
efa1afe
Start adding the side effect support classes to the interaction model…
andy31415 Apr 9, 2024
4eeb04f
Add actions and integrate them to the IM model
andy31415 Apr 9, 2024
f4c3b46
Add iteration methods
andy31415 Apr 9, 2024
4abcb5e
Restyle
andy31415 Apr 9, 2024
34b646c
Add all files to the build definition
andy31415 Apr 9, 2024
02bc810
Start defining a unit test to at least validate compilation. Will lat…
andy31415 Apr 9, 2024
05a085a
Fix a dependency
andy31415 Apr 9, 2024
dfab9de
Move EventLoggingDelegate to be part of "events" in app.
andy31415 Apr 9, 2024
43308e8
Prepare for testing ... does not compile however TODOs are starting t…
andy31415 Apr 9, 2024
fa4cfa8
Fix SFINAE logic on eventing
andy31415 Apr 9, 2024
7fb1a3c
test that something is actually emitted for events
andy31415 Apr 9, 2024
e97254b
Tests actually do something
andy31415 Apr 9, 2024
f855e93
Some tests actually pass
andy31415 Apr 9, 2024
0aa318a
Restyle
andy31415 Apr 9, 2024
7e95ee7
Merge branch 'master' into im_dm_decoupling
andy31415 Apr 9, 2024
6e532a6
Minor style updates
andy31415 Apr 9, 2024
4c3391a
Merge branch 'master' into im_dm_decoupling
andy31415 Apr 11, 2024
0a0b315
Rename EmitEvent to GenerateEvent
andy31415 Apr 11, 2024
eb58bc9
Add description for paths
andy31415 Apr 11, 2024
d88fb0f
Cleanup some example text ... we do not need an exhaustive usage list
andy31415 Apr 11, 2024
acc842c
Restyle
andy31415 Apr 11, 2024
6886d0c
Fix spelling
andy31415 Apr 11, 2024
6c805d1
Renames based on code review: use UpperCamelCase
andy31415 Apr 11, 2024
bc5f9f6
Updated proposal for retries and paths
andy31415 Apr 11, 2024
b9db782
Restyle
andy31415 Apr 11, 2024
24176e9
one more comment
andy31415 Apr 11, 2024
cff6de1
Restyled by clang-format
restyled-commits Apr 11, 2024
7310b65
Fix typo in parameter type
andy31415 Apr 12, 2024
7d99ad5
Some updates for code review
andreilitvin Apr 16, 2024
c363ddd
Merge branch 'master' into im_dm_decoupling
andreilitvin Apr 16, 2024
c925cb4
Undo submodule update
andreilitvin Apr 16, 2024
72ea3cc
Fix logic error in buffer-too-small check for buffer data
andreilitvin Apr 16, 2024
1da1c87
Fix return error code when send encounters an error
andreilitvin Apr 16, 2024
036782f
Fix logic for mCompleted handling in the auto-complete handling
andreilitvin Apr 16, 2024
ddbe6a6
More comments
andreilitvin Apr 16, 2024
f3673ba
Comments
andreilitvin Apr 16, 2024
b220cf3
Use AAI types for interaction model. I think they will need some spli…
andreilitvin Apr 16, 2024
c2d2935
Update some naming to not use the a... syntax since those are odd
andreilitvin Apr 16, 2024
af649e2
Merge branch 'master' into im_dm_decoupling
andy31415 Apr 25, 2024
0c9cd9e
Code review updates
andy31415 Apr 25, 2024
7856b2e
Added extra comment
andy31415 Apr 25, 2024
e9c0039
Enforce lifetime in invoke responses
andy31415 Apr 25, 2024
e6cdc3e
More comments
andy31415 Apr 25, 2024
7a12efd
More comments
andy31415 Apr 25, 2024
8c86e3d
Restyle
andy31415 Apr 25, 2024
e4b0010
Added comment on replyasync with nullptr
andy31415 Apr 25, 2024
533d542
Update src/app/interaction-model/InvokeResponder.h
andy31415 Apr 25, 2024
4be818f
Update src/app/interaction-model/InvokeResponder.h
andy31415 Apr 25, 2024
492b6f9
Update src/app/interaction-model/InvokeResponder.h
andy31415 Apr 25, 2024
5f76b9c
Update src/app/interaction-model/InvokeResponder.h
andy31415 Apr 25, 2024
7e18454
Some code review updates
andreilitvin Apr 26, 2024
5844827
Some code review updates
andreilitvin Apr 26, 2024
e5e97a8
Merge branch 'im_dm_decoupling' of github.com:andy31415/connectedhome…
andreilitvin Apr 26, 2024
89c5bcf
Make the subject descriptor optional
andreilitvin Apr 26, 2024
ceea7b8
use std::optional instead of chip optiona
andreilitvin Apr 26, 2024
d134fab
Restyle
andreilitvin Apr 26, 2024
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
1 change: 1 addition & 0 deletions src/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ if (chip_build_tests) {
chip_test_group("tests") {
deps = []
tests = [
"${chip_root}/src/app/interaction-model/tests",
"${chip_root}/src/access/tests",
"${chip_root}/src/crypto/tests",
"${chip_root}/src/inet/tests",
Expand Down
37 changes: 37 additions & 0 deletions src/app/interaction-model/Actions.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright (c) 2024 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#pragma once

#include <app/interaction-model/Events.h>
#include <app/interaction-model/Paths.h>
#include <app/interaction-model/RequestContext.h>

namespace chip {
namespace app {
namespace InteractionModel {

/// Actual provided data for data models to interface with the interaction model environment
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
struct InteractionModelActions
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a problem: "Action" in the context of the interaction model has a specific meaning, and this struct has absolutely nothing to do with that meaning.

I can't tell by looking at this struct name, or the struct definition, what is supposed to go into here and why it has, say, Events but not Attributes or Commands. And I have no idea what Paths is supposed to be. If it's "things you can do with paths", that seems like the wrong level of abstraction to me.

This is really "InteractionModelAPIs" or something. But then broken down into separate structs for separate types of APIs... (more on this below).

Realistically, this should perhaps have been called chip::app::InteractionModel (as a class, not a namespace), but if we are using that name for the namespace.... chip::app::InteractionModel::Instance? chip::app::InteractionModel::Callbacks? Something else? I am struggling to find a sensible name here.

{
Events * events;
Paths * paths;
RequestContext * requestContext;
};

} // namespace InteractionModel
} // namespace app
} // namespace chip
41 changes: 41 additions & 0 deletions src/app/interaction-model/BUILD.gn
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Copyright (c) 2024 Project CHIP Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import("//build_overrides/chip.gni")

source_set("interaction-model") {
sources = [
"Actions.h",
"Events.h",
"InvokeResponder.h",
"IterationTypes.h",
"Model.h",
"OperationTypes.h",
"Paths.h",
"RequestContext.h",
]

public_deps = [
"${chip_root}/src/access:types",
"${chip_root}/src/app:attribute-access",
"${chip_root}/src/app:events",
"${chip_root}/src/app:paths",
"${chip_root}/src/app/MessageDef",
"${chip_root}/src/app/data-model",
"${chip_root}/src/lib/core",
"${chip_root}/src/lib/core:error",
"${chip_root}/src/lib/core:types",
"${chip_root}/src/lib/support",
"${chip_root}/src/messaging",
]
}
130 changes: 130 additions & 0 deletions src/app/interaction-model/Events.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
/*
* Copyright (c) 2024 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#pragma once

#include <app/EventLoggingDelegate.h>
#include <app/EventLoggingTypes.h>
#include <app/MessageDef/EventDataIB.h>
#include <app/data-model/Encode.h>
#include <app/data-model/FabricScoped.h>
#include <lib/core/CHIPError.h>
#include <lib/support/logging/CHIPLogging.h>

#include <type_traits>

namespace chip {
namespace app {
namespace InteractionModel {

namespace internal {
template <typename T>
class SimpleEventLoggingDelegate : public EventLoggingDelegate
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
{
public:
SimpleEventLoggingDelegate(const T & aEventData) : mEventData(aEventData){};
CHIP_ERROR WriteEvent(chip::TLV::TLVWriter & aWriter) final override
{
return DataModel::Encode(aWriter, TLV::ContextTag(EventDataIB::Tag::kData), mEventData);
}

private:
const T & mEventData;
};

template <typename E, typename T, std::enable_if_t<DataModel::IsFabricScoped<T>::value, bool> = true>
EventNumber GenerateEvent(E & emittor, const T & aEventData, EndpointId aEndpoint)
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
{
internal::SimpleEventLoggingDelegate<T> eventData(aEventData);
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
ConcreteEventPath path(aEndpoint, aEventData.GetClusterId(), aEventData.GetEventId());
EventOptions eventOptions;
eventOptions.mPath = path;
eventOptions.mPriority = aEventData.GetPriorityLevel();
eventOptions.mFabricIndex = aEventData.GetFabricIndex();

// this skips logging the event if it's fabric-scoped but no fabric association exists yet.
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved

if (eventOptions.mFabricIndex == kUndefinedFabricIndex)
{
ChipLogError(EventLogging, "Event encode failure: no fabric index for fabric scoped event");
return kInvalidEventId;
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
}

//
// Unlike attributes which have a different 'EncodeForRead' for fabric-scoped structs,
// fabric-sensitive events don't require that since the actual omission of the event in its entirety
// happens within the event management framework itself at the time of access.
//
// The 'mFabricIndex' field in the event options above is encoded out-of-band alongside the event payload
// and used to match against the accessing fabric.
//
EventNumber eventNumber;
CHIP_ERROR err = emittor.GenerateEvent(&eventData, eventOptions, eventNumber);
if (err != CHIP_NO_ERROR)
{
ChipLogError(EventLogging, "Failed to log event: %" CHIP_ERROR_FORMAT, err.Format());
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
return kInvalidEventId;
}

return eventNumber;
}

template <typename E, typename T, std::enable_if_t<!DataModel::IsFabricScoped<T>::value, bool> = true>
EventNumber GenerateEvent(E & emittor, const T & aEventData, EndpointId endpointId)
{
internal::SimpleEventLoggingDelegate<T> eventData(aEventData);
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
ConcreteEventPath path(endpointId, aEventData.GetClusterId(), aEventData.GetEventId());
EventOptions eventOptions;
eventOptions.mPath = path;
eventOptions.mPriority = aEventData.GetPriorityLevel();
EventNumber eventNumber;
CHIP_ERROR err = emittor.GenerateEvent(&eventData, eventOptions, eventNumber);
if (err != CHIP_NO_ERROR)
{
ChipLogError(EventLogging, "Failed to log event: %" CHIP_ERROR_FORMAT, err.Format());
return kInvalidEventId;
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
}

return eventNumber;
}

} // namespace internal

class Events
Copy link
Contributor

Choose a reason for hiding this comment

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

So this class name... Is this intended to be a grab bag of "all APIs related to events for a given IM implementation" or something?

The naming here is pretty opaque as an API consumer; what does it mean to have an Events * that does not actually give me access to any events, for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a hard time naming overall. What I have is that various things made their way into our data model implementation in ember without explicitly being named or sorted in categories - they moved organically as "we neeed this" and it seems to typically carry over either as internal access (e.g. exchange-context -> session -> groupid) or as "implied global" (this is what events do) and this is what makes decoupling extra hard - I really don't want to use implied globals.

I tried to split them as:

  • paths (because marking a path dirty seems to be a required thing)
  • events (code needs to be able to report/generate events)
  • Actions (now renamed to Context) as a I give up ... some code really needs access to internals, specifically GetExchangeContext

I would welcome suggestions of good names. This seems to be some EventContext however naming everything Context seems odd ... and I am torn about context being a filler word or not.

Maybe the correct thing would be to place these under a Context or Callback namespace, however since I could not find a good name I changed it to "ok for now". Willing to update though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think "paths" is very weird because there are lots of types of paths. Do "event paths" go under Paths or under Events?

{
public:
virtual ~Events() = default;

/// Generates the given event.
///
/// Events are generally expected to be sent to subscribed clients and also
/// be available for read later until they get overwritten by new events
/// that are being generated.
virtual CHIP_ERROR GenerateEvent(EventLoggingDelegate * eventContentWriter, const EventOptions & options,
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
EventNumber & generatedEventNumber) = 0;

// Convenience methods for event logging using cluster-object structures
// On error, these log and return kInvalidEventId
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
template <typename T>
EventNumber GenerateEvent(const T & eventData, EndpointId endpointId)
{
return internal::GenerateEvent(*this, eventData, endpointId);
}
};

} // namespace InteractionModel
} // namespace app
} // namespace chip
Loading
Loading