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

Add activity callback function #264

Merged
merged 3 commits into from
Dec 26, 2023

Conversation

Ing-Dom
Copy link
Contributor

@Ing-Dom Ing-Dom commented Dec 17, 2023

Add the possibility to have a callback function signaling knx activity (for e.g. flashing LEDs)

Can be enabled with KNX_ACTIVITYCALLBACK

…y (for e.g. flashing LEDs)

Can be enabled with KNX_ACTIVITYCALLBACK

# Conflicts:
#	src/knx/ip_data_link_layer.cpp
@thelsing
Copy link
Owner

For achitectional reasons I will not merge this. The knx-Facade implements the facade pattern an simplifies usage for lib users. It is completely optional. Therefore you can't use it in any of the other parts of the stack. For the same reason its global instance must not be used in the stack.

The correct way is to add the callback to be Bau. There can be multiple Baus (in routers for example) and each can have its own callback.

Copy link
Owner

@thelsing thelsing left a comment

Choose a reason for hiding this comment

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

See comment on PR:

@Ing-Dom
Copy link
Contributor Author

Ing-Dom commented Dec 20, 2023

yes you are right.
I'll rework it.
do you really mean bau, because the router baus in the stack are "single"
I think you mean more then one link layer do you, like in the bau091A ? (or the other coupler baus)

…ore flexible approach.

For performace reasons, the calls in the actual link layer is only activated when KNX_ACTIVITYCALLBACK is defined.

There is now a DataLinkLayerCallback class where a specific Bau *can* inherit from and - if the specific link layer supports it, pass itself to that linklayer(s).
@Ing-Dom
Copy link
Contributor Author

Ing-Dom commented Dec 25, 2023

Hi Thomas,

I redesigned the feature according you hints.

@thelsing
Copy link
Owner

I mean more than one logical device in one physical device. My ip router for example has one PA for the router application and a different one for additional stuff like providing time to the bus. The facade was never meant for bigger stuff like openknx with people who know how the stack is structured, but for simple sketches for typical arduino users.

Copy link
Owner

@thelsing thelsing left a comment

Choose a reason for hiding this comment

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

This version is fine beside some minor style issues. Sorry for being nitpicky but I have to work with code at work were people weren't for a few years...

src/knx/data_link_layer.h Outdated Show resolved Hide resolved
src/knx/ip_data_link_layer.cpp Outdated Show resolved Hide resolved
src/knx/tpuart_data_link_layer.cpp Outdated Show resolved Hide resolved
src/knx/tpuart_data_link_layer.h Outdated Show resolved Hide resolved
@Ing-Dom
Copy link
Contributor Author

Ing-Dom commented Dec 26, 2023

I mean more than one logical device in one physical device. My ip router for example has one PA for the router application and a different one for additional stuff like providing time to the bus. The facade was never meant for bigger stuff like openknx with people who know how the stack is structured, but for simple sketches for typical arduino users.

I work on an ip router (091A) based on this stack for alomost one year (PR will come if its a little more field tested).
For this I thought about how to solve the second application (virtual knx device) but I think that will not be so easy - some sort of bus-access splitter (or however you call it) would be neccessary, right?

Copy link
Owner

@thelsing thelsing left a comment

Choose a reason for hiding this comment

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

All ok now.

@thelsing
Copy link
Owner

I work on an ip router (091A) based on this stack for alomost one year (PR will come if its a little more field tested). For this I thought about how to solve the second application (virtual knx device) but I think that will not be so easy - some sort of bus-access splitter (or however you call it) would be neccessary, right?

For knx-ip you should be able open more than one multicast connection from one host and use them in parallel.

@thelsing thelsing merged commit 7215f47 into thelsing:master Dec 26, 2023
7 checks passed
@Ing-Dom Ing-Dom deleted the feature_activitycallback branch January 2, 2024 15:37
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.

2 participants