Skip to content
This repository has been archived by the owner on Feb 5, 2024. It is now read-only.

Xbees comms #161

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from

Conversation

SnackkOverflowError
Copy link
Contributor

adding comms from plane to ground

this is a draft because I need to fix style and also add comments, but as far as I can tell, it works.

code in a working state, the previous crap was shitty
@SnackkOverflowError
Copy link
Contributor Author

Might need some fixes to style still or commenting, but the code is in a working state now.

@SnackkOverflowError SnackkOverflowError marked this pull request as ready for review January 31, 2022 07:44
Copy link
Contributor

@upadhyaydhruv upadhyaydhruv left a comment

Choose a reason for hiding this comment

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

Thanks for getting this done! I've added some comments, mainly questions about how certain things work. Ultimately, would love some documentation on the air-ground comms as well!



class pogiData {
public:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we fix the indentation here to make it more readable?

Copy link
Member

Choose a reason for hiding this comment

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

(Yes tab after both public: and private:)

Autopilot/Inc/comms.hpp Show resolved Hide resolved
uint8_t checksum;
};

struct responseFrame {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of the responseFrame?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no purpose currently, but when you send an outgoing message, the xbee will send you a response telling you if it failed or succeeded. Im working on support for that in v2 which ill be done with soon.

Autopilot/Inc/comms.hpp Show resolved Hide resolved
Autopilot/Src/Callbacks.cpp Show resolved Hide resolved
}

void pogiData::setPMState(uint8_t state) {
this->statusDisplay = (this->statusDisplay & 01110000) | (state & 00001111);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this statusDisplay & 11110000? If you're really only setting the last four bits, as you do with state & 00001111?

void Comms::transmitMessage() {
transmitFrame frame;
frame.payload = this->tx;
frame.id = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

what does id signify?

uint8_t rawData[40] = { 0 };
uint8_t checksum = 0xFF;

rawData[0] = frame.startDelim;
Copy link
Contributor

Choose a reason for hiding this comment

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

are you basically going through your transmit frame, putting it into an array of uint8_t, and transmitting that instead of the whole struct?

uint8_t size = sizeof(rawData);

// send the data
HAL_UART_Transmit(&huart1, rawData, size,100);
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered doing what Yashraj did in the UART driver for FW-CV comms? He is able to reinterpret and transmit structs effectively. That might be a better solution

@@ -80,7 +82,7 @@ void MX_USART2_UART_Init(void)
{

huart2.Instance = USART2;
huart2.Init.BaudRate = 115200;
huart2.Init.BaudRate = 9600;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this what the XBees use? No way to increase it somehow?

Autopilot/Inc/comms.hpp Show resolved Hide resolved
Autopilot/Inc/comms.hpp Show resolved Hide resolved
public:
uint8_t outputs[4] = { 0 };
uint8_t grabberPos = 0;
// 5
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed? If so then make this comment clearer (eg. Byte offset: 5)

Comment on lines +42 to +43
void setControlState(uint8_t state); // 2 bits
void setPMState(uint8_t state); // 4 bits
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to make it clearer the data format going into these functions?



class pogiData {
public:
Copy link
Member

Choose a reason for hiding this comment

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

(Yes tab after both public: and private:)

}
if(huart == &huart2) {
HAL_UART_Receive_DMA(&huart2, Comms::GetInstance()->rxBuffer, 40);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

add a line at the end of the file to make github happy

Comms* Comms::instance = nullptr;

Comms::Comms() {
//constructor
Copy link
Member

Choose a reason for hiding this comment

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

Can remove this comment

}

// this is a weird dumb no good way to do this, but I am tired and this works
// manually unwrapping the class because a cast seems to not work (internet says that classes have padding)
Copy link
Member

Choose a reason for hiding this comment

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

Often there will be an additional 8 bit address associated with pointers, but if this works, that can be looked at later


rawData[0] = frame.startDelim;

uint8_t *castedVal = (uint8_t*)&frame.length;
Copy link
Member

Choose a reason for hiding this comment

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

castVal? Casted feels like bad grammar

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants