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

Added GEA2 Communications #4

Merged
merged 32 commits into from
Dec 9, 2024
Merged

Conversation

KaitlynAnn99
Copy link
Contributor

@KaitlynAnn99 KaitlynAnn99 commented Nov 25, 2024

Adding GEA2 communications to the interface.

README.md Outdated
@@ -10,6 +10,9 @@ Provides a simple interface for sending and receiving GEA3 serial packets.
### `tiny_erd_client`
Provides a simple interface for reading and writing addressable data (ERDs) over a GEA3 serial interface.

## `tiny_gea2_interface_single_wire`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## `tiny_gea2_interface_single_wire`
### `tiny_gea3_interface_single_wire`

Can you put this up by the other interface?

@ryanplusplus
Copy link
Contributor

The filename for the new component are inconsistent, can you get them all to match?

Copy link

@brownralex brownralex left a comment

Choose a reason for hiding this comment

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

Mainly just questions but I am curious why you organized the fsm this way. Why not put all the states together and put all the helper functions above it? Is this just standard practice?


static void byte_received(void* context, const void* _args)
{
reinterpret(instance, context, self_t*);

Choose a reason for hiding this comment

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

So I remember talking to Andrew and he said that we should avoid using reinterprets. Is that not the case here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I went ahead and removed these

tiny_fsm_send_signal(&instance->_private.fsm, signal_reflection_timeout, NULL);
}

static bool determine_byte_to_send_considering_escapes(self_t* instance, uint8_t byte, uint8_t* byteTosend)

Choose a reason for hiding this comment

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

What does it mean to have a byte escape

Copy link
Contributor Author

@KaitlynAnn99 KaitlynAnn99 Nov 25, 2024

Choose a reason for hiding this comment

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

The etx byte is the final byte of the protocol. It basically says the message is done now

edit: my bad Idk why i thought you were talking about the etx and not escape

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, escape is how we distinguish between control characters acting as control characters (for example, start of frame, end of frame) and data that happens to look like control characters. So as an example, if we have a byte that matches STX in the middle of the payload, we'll prefix it with an ESC byte so that the receiver knows it's not really an STX control character, it's data.

break;

case send_state_data:
if(determine_byte_to_send_considering_escapes(instance, instance->_private.send.buffer[instance->_private.send.offset], &byteTosend)) {

Choose a reason for hiding this comment

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

Going with my last comment but why would something be escaping from the rest of the data?

{
uint8_t byteTosend = 0;

tiny_timer_start(

Choose a reason for hiding this comment

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

I noticed that this timer is meant to signal that there was a failure in the send but it doesn't seem like anything stops the timer if the next byte is sent. Is there assumption that there will always be failture?

collision_idle_timeout);
}

static void state_collision_cooldown(tiny_fsm_t* fsm, const tiny_fsm_signal_t signal, const void* data)

Choose a reason for hiding this comment

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

What are collisions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so this is a single wire protocol - there is one data line. If something tries to send a message at the same time as the other one, the messages "collide" and we need to wait for a cooldown period to finish before attempting to send again

README.md Outdated Show resolved Hide resolved
@ryanplusplus ryanplusplus merged commit b0d379a into geappliances:develop Dec 9, 2024
5 checks passed
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