-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add float to int conversion directives, some misc changes #3847
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
Conversation
| this->log_WARNING_HI_FileReadDeserializeError(this->m_sequenceFilePath, static_cast<I32>(deserStatus), | ||
| this->m_sequenceBuffer.getBuffLeft(), | ||
| this->m_sequenceBuffer.getBuffLength()); | ||
| this->log_WARNING_HI_FileReadDeserializeError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just added the filereadstage to these events
| } | ||
|
|
||
| readStatus = readBytes(sequenceFile, this->m_sequenceObj.getheader().getbodySize()); | ||
| readStatus = |
Check warning
Code scanning / CodeQL
Unchecked return value Warning
operator=
|
|
||
| // read footer bytes but don't include in CRC | ||
| readStatus = this->readBytes(sequenceFile, Fpy::Footer::SERIALIZED_SIZE, false); | ||
| readStatus = this->readBytes(sequenceFile, Fpy::Footer::SERIALIZED_SIZE, FpySequencer_FileReadStage::FOOTER, false); |
Check warning
Code scanning / CodeQL
Unchecked return value Warning
operator=
|
|
||
| if (fileStatus != Os::File::OP_OK) { | ||
| this->log_WARNING_HI_FileReadError(this->m_sequenceFilePath, static_cast<I32>(fileStatus)); | ||
| this->log_WARNING_HI_FileReadError(readStage, this->m_sequenceFilePath, static_cast<I32>(fileStatus)); |
Check warning
Code scanning / CodeQL
Unchecked function argument Warning
|
|
||
| if (actualReadLen < expectedReadLen) { | ||
| this->log_WARNING_HI_EndOfFileError(this->m_sequenceFilePath); | ||
| this->log_WARNING_HI_EndOfFileError(readStage, this->m_sequenceFilePath); |
Check warning
Code scanning / CodeQL
Unchecked function argument Warning
| // reads some bytes from the open file into the m_sequenceBuffer. | ||
| // return success if successful | ||
| Fw::Success FpySequencer::readBytes(Os::File& file, FwSizeType expectedReadLen, bool updateCrc) { | ||
| Fw::Success FpySequencer::readBytes(Os::File& file, |
Check notice
Code scanning / CodeQL
Long function without assertion Note
| // this sequence caught one bug | ||
| allocMem(); | ||
|
|
||
| add_SET_REG(0, 255); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the previous NOT behavior, this test would fail. now it succeeds
| I64 FpySequencer::unaryRegOp_fptosi(I64 src) { | ||
| // first interpret as F64 | ||
| F64 fsrc; | ||
| memcpy(&fsrc, &src, sizeof(fsrc)); |
Check warning
Code scanning / CodeQL
Unchecked return value Warning
memcpy
| I64 FpySequencer::unaryRegOp_fptosi(I64 src) { | ||
| // first interpret as F64 | ||
| F64 fsrc; | ||
| memcpy(&fsrc, &src, sizeof(fsrc)); |
Check warning
Code scanning / CodeQL
Unchecked function argument Warning
| } | ||
| I64 FpySequencer::unaryRegOp_sitofp(I64 src) { | ||
| // first static cast to float | ||
| F64 fsrc = static_cast<F64>(src); |
Check warning
Code scanning / CodeQL
Unchecked function argument Warning
| F64 fsrc = static_cast<F64>(src); | ||
| // then return bits as I64 | ||
| I64 res; | ||
| memcpy(&res, &fsrc, sizeof(res)); |
Check warning
Code scanning / CodeQL
Unchecked return value Warning
memcpy
| I64 FpySequencer::unaryRegOp_fptoui(I64 src) { | ||
| // first interpret as F64 | ||
| F64 fsrc; | ||
| memcpy(&fsrc, &src, sizeof(fsrc)); |
Check warning
Code scanning / CodeQL
Unchecked return value Warning
memcpy
| F64 fsrc = static_cast<F64>(static_cast<U64>(src)); | ||
| // then return bits as I64 | ||
| I64 res; | ||
| memcpy(&res, &fsrc, sizeof(res)); |
Check warning
Code scanning / CodeQL
Unchecked return value Warning
memcpy
| res = this->unaryRegOp_fptrunc(src); | ||
| break; | ||
| case Fpy::DirectiveId::FPTOSI: | ||
| res = this->unaryRegOp_fptosi(src); |
Check warning
Code scanning / CodeQL
Unchecked return value Warning
unaryRegOp_fptosi
| res = this->unaryRegOp_fptosi(src); | ||
| break; | ||
| case Fpy::DirectiveId::FPTOUI: | ||
| res = this->unaryRegOp_fptoui(src); |
Check warning
Code scanning / CodeQL
Unchecked return value Warning
unaryRegOp_fptoui
| res = this->unaryRegOp_fptoui(src); | ||
| break; | ||
| case Fpy::DirectiveId::SITOFP: | ||
| res = this->unaryRegOp_sitofp(src); |
Check warning
Code scanning / CodeQL
Unchecked return value Warning
unaryRegOp_sitofp
| res = this->unaryRegOp_sitofp(src); | ||
| break; | ||
| case Fpy::DirectiveId::UITOFP: | ||
| res = this->unaryRegOp_uitofp(src); |
Check warning
Code scanning / CodeQL
Unchecked return value Warning
unaryRegOp_uitofp
| memcpy(&fsrc, &src, sizeof(fsrc)); | ||
| // then static cast to unsigned int | ||
| // then return as a signed int | ||
| return static_cast<I64>(static_cast<U64>(fsrc)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this technically not do anything? probably yes... it's more explicit about what's actually going on here though. The float is supposed to be converted to a U64, and then it's supposed to be represented in the bits of an I64... to be honest, the CPP standard is confusing and I'm not sure how best to write this. It's all quite subtle...
LeStarch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had one clarifications. Otherwise looks good.
I will note the I64 representing all registers is getting very hard to follow. Memcpys just seem wrong. I know why it is done, but I am starting to think there must be a better way.
| } | ||
| I64 FpySequencer::unaryRegOp_not(I64 src) { | ||
| return ~src; | ||
| if (src) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed this would be a biteize not, and it seems to be a logical not. This maintains the weirdness that logical "not" can be used on non-booleans using C++ behavior. Is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes--the NOT instruction would theoretically work if you passed in a non boolean. However, at the high level language we disallow this--you can only take the NOT of a boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also consider, at the bytecode level, we don't really know what "is" or "isn't" a boolean--it's all just a series of 8 byte registers of unspecified type.
There is a better way, and it's the next thing I'm going to work on! Switching over to a stack-based architecture will mean that there won't be any registers any more. That's coming, hopefully before the design review. |
LeStarch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I buy the argument
Change Description
Adds the FPTOI (floating point to integer) and ITOFP directives, which take 64 bit ints and floats and convert between them as if by static cast.
I also added a small amount of debugging telemetry to certain deserialization/file read errors. This was helpful to me for debugging problems and I think would be helpful to users too.
I also updated the default register count to 128. This equates to 1024 bytes of memory used for registers. Without lots of registers, many sequences do not run, because currently I do not perform any register allocation optimization. In the next large Fpy update I will remove the concept of statically allocated registers and replace them with a stack, so this constant will go away. For now, in order to be usable out of the box, the register count needs to be high.
I also fixed a bug in the NOT directive. Previously, it was implemented to take the bitwise complement of whatever was given to it. However, this produced incorrect behavior when trying to get the inverse of the Fprime "TRUE" value (0xFF == 255), as it would result in -256 (0xFFFFFFFFFFFFFF00). Now, it interprets the input register as a C++ boolean, and if true it outputs
static_cast<I64>(false), otherwisetrue. This gives the correct behavior under all situations.Rationale
This will allow users of Fpy to compare and perform math where one operand is a float and the other is an int. Otherwise, you wouldn't be able to check if
5.0 > 4.